credo icon indicating copy to clipboard operation
credo copied to clipboard

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

Open elridion opened this issue 1 year ago • 3 comments

Environment

  • Credo version: 1.7.5-ref.main.81cc50fba+uncommittedchanges
  • Erlang/Elixir version: Erlang/OTP 26 [erts-14.1] [source] [64-bit] [smp:10:10] [ds:10:10:10] [async-threads:1] [jit] Elixir 1.15.6 (compiled with Erlang/OTP 26)
  • Operating system: OSX

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

  • Enum.concat(list_so_far, [new_item]) or ...
  • List.insert_at(list_so_far, -1, new_item)

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

elridion avatar May 07 '24 15:05 elridion

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! ✌️

rrrene avatar May 09 '24 11:05 rrrene