exercism / elixir-analyzer

GNU Affero General Public License v3.0
30 stars 32 forks source link

Analyzer for boutique-inventory is missing one possible solution avoiding Enum.map for increase_quantity() #357

Closed rmonnet closed 1 year ago

rmonnet commented 1 year ago

The analyzer states that Map.new/2 or Enum.into/3 should be used to avoid Enum.Map/2. There is an alternate solution using a comprehension with the option 'into: %{}' that doesn't use Map.new or Enum.into and still avoid Enum.Map:

  def increase_quantity(item, count) do
    updated_quantities = 
      for {k, v} <- item[:quantity_by_size], into: %{}, do: {k, v + count}
    Map.put(item, :quantity_by_size, updated_quantities)
  end

Currently, the analyzer complains about the code above and I have to submit the following to avoid the warning.

  def increase_quantity(item, count) do
    updated_quantities = 
      for {k, v} <- item[:quantity_by_size] do: {k, v + count}
    Map.put(item, :quantity_by_size, Map.new(updated_quantities))
  end
jiegillet commented 1 year ago

Hi @rmonnet, The goal of boutique-inventory is to learn about Map and this analyzer check was made with the explicit intent of nudging students towards one of the following solutions:

      def increase_quantity(item, count) do
        Map.update(item, :quantity_by_size, %{}, fn quantity_by_size ->
            Map.new(quantity_by_size, fn {size, quantity} -> {size, quantity + count} end)
        end)
      end

      def increase_quantity(item, count) do
        Map.update(item, :quantity_by_size, %{}, fn quantity_by_size ->
          Enum.into(quantity_by_size, %{}, fn {size, quantity} -> {size, quantity + count} end)
        end)
      end

There are of course, alternate solution to get to the same result, like yours, but we felt that those two are particularly good and are worth highlighting, so I would say the analyzer works an intended. What do you think?

rmonnet commented 1 year ago

I guess I missed the point that the exercise is about Map. In that case, the analyzer's comment makes sense. It is interesting that the analyzer comments on Map.new and Enum.into not being used but is silent on the fact that I missed to use Map.update (the point of the exercise).

Thank you for the feedback.

On Fri, Dec 23, 2022 at 6:53 AM Jie @.***> wrote:

Hi @rmonnet https://github.com/rmonnet, The goal of boutique-inventory is to learn about Map and this analyzer check was made with the explicit intent of nudging students towards one of the following solutions:

  def increase_quantity(item, count) do
    Map.update(item, :quantity_by_size, %{}, fn quantity_by_size ->
        Map.new(quantity_by_size, fn {size, quantity} -> {size, quantity + count} end)
    end)
  end

  def increase_quantity(item, count) do
    Map.update(item, :quantity_by_size, %{}, fn quantity_by_size ->
      Enum.into(quantity_by_size, %{}, fn {size, quantity} -> {size, quantity + count} end)
    end)
  end

There are of course, alternate solution to get to the same result, like yours, but we felt that those two are particularly good and are worth highlighting, so I would say the analyzer works an intended. What do you think?

— Reply to this email directly, view it on GitHub https://github.com/exercism/elixir-analyzer/issues/357#issuecomment-1363889251, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAICHA2S2EJFLQRSE6QMQX3WOWHDBANCNFSM6AAAAAATHCIBVM . You are receiving this because you were mentioned.Message ID: @.***>

jiegillet commented 1 year ago

Well, there are many, many things to learn from the Enum related modules. Map.new and Enum.into happen to be mentioned in the concept introduction, so we are pushing these, but your solution using Map.put is good too. We gotta pick our battles :) Thanks for the conversation!