rrrene / credo

A static code analysis tool for the Elixir language with a focus on code consistency and teaching.
http://credo-ci.org/
MIT License
4.91k stars 414 forks source link

Arguable advice from `Credo.Check.Refactor.AppendSingleItem` #1129

Open elridion opened 4 months ago

elridion commented 4 months ago

Environment

The issue

The Credo.Check.Refactor.AppendSingleItem correctly flags a bunch of places where singles items are in fact appended to lists and that's all good. Example from credo list = list_so_far ++ [new_item]

The issue is that the example states to use Enum.reverse/1 when order matters and gives the following adivce:

list = [new_item] ++ list_so_far
# ...
Enum.reverse(list)

The thing is that the given solution is somewhat wrong. If order matters and the list_so_far is something like [1, 2, 3] and we try to append 4 the resulting list looks like this[3, 2, 1, 4].

The correct solutions would be:

list = [new_item] ++ Enum.reverse(list_so_far)
# ...
Enum.reverse(list)

But I would argue that ist in fact messier than before.

Additionally

To add to that two possible functions come to mind to get around using ++ there would be

the thing is both are using the ++ themself. See Enum.concat and List.insert_at

rrrene commented 3 months ago

While I think that I am getting what you are trying to say, I am not sure what you suggest to do about it.

I understand why you are saying that the example given might be misunderstood. Yet, this hasn't come up in the 7 years this check exists.

If you have a suggestion for a better phrasing of the check's description, please share! ✌️