elixir-analyzer icon indicating copy to clipboard operation
elixir-analyzer copied to clipboard

Accept defdelegates as valid equivalents of function calls in `assert_call`

Open TBK145 opened this issue 2 years ago • 2 comments

Hi, I'd like to start off with saying I love this tool and the exercises. Now I've been using Elixir for a couple of years already and I'm kinda trying to work my way through the problems as fast as possible, and I implemented the "High Score' problem using defdelegate. Now I understand this isn't the point of the exercise, but the analysis remarks I got weren't correct.

This is the code:

defmodule HighScore do
  @default_score 0
  defdelegate new, to: Map
  defdelegate add_player(scores, name, score \\ @default_score), to: Map, as: :put
  defdelegate remove_player(scores, name), to: Map, as: :delete
  defdelegate reset_score(scores, name), to: __MODULE__, as: :add_player
  def update_score(scores, name, score), do: Map.update(scores, name, score, fn x -> score + x end)
  defdelegate get_players(scores), to: Map, as: :keys
end

This was the feedback of the analysis:

Essential Use a module attribute in add_player/3 and reset_score/2 as a constant for the initial score. Recommended Use a default argument in the add_player/3 function to handle a lack of initial score. Use the module attribute with the default initial score.

Even though the defdelegates are probably cheating, I am using default arguments and a module attribute, so IMO the analysis is not correct. Maybe it's an option to instead add feedback to not use defdelegate as well, as it defeats the purpose of the exercise.

TBK145 avatar Jun 02 '22 12:06 TBK145

TIL about defdelegate :) I wouldn't expect beginners to know about it either, but that doesn't mean it's cheating to use it. That being said, it might be a little tricky to fix the essential comment, so I think I would go with your last suggestion.

jiegillet avatar Jun 04 '22 06:06 jiegillet

Hi!

First of all, I'm really sorry it took me so long to get back to you 🙏. It has been a very busy summer for me.

Second of all, thank you for reporting this and bringing this issue to our attention.

I'm not entirely sure if I agree with this statement:

not use defdelegate as well, as it defeats the purpose of the exercise.

I think a solution with defdelegates like yours is perfectly valid. You still had to choose all the right Map module functions and know how to use them. The fact that you used them via defdelegate instead of good old function calls doesn't make a difference here in my opinion. For this reason, I would be against telling students not to use it.

However, as @jiegillet pointed out, fixing the analyzer to accept defdelegates as valid equivalents of function calls might be a bit tricky, but I still think that's the right course of action. I'll attempt it when I have a bit more time.

For now, a quick fix could be to ignore all assert_calls for this exercise if there's any defdelegate present. There's a suppress_if option in our custom macros that should make this possible.

angelikatyborska avatar Jul 15 '22 10:07 angelikatyborska

This is still not working correctly. I submitted a solution almost identical to this one, and the "default argument" feedback still appears.

bugQ avatar Mar 08 '23 01:03 bugQ

@bugQ Can you please copy-paste your solution so we can check?

angelikatyborska avatar Mar 08 '23 06:03 angelikatyborska

@angelikatyborska

defmodule HighScore do
  @default_score 0

  def new(), do: %{}

  defdelegate add_player(scores, name, score \\ @default_score), to: Map, as: :put

  defdelegate remove_player(scores, name), to: Map, as: :delete

  def reset_score(scores, name), do: Map.put(scores, name, @default_score)

  def update_score(scores, name, score), do: Map.update(scores, name, score, fn x -> x + score end)

  defdelegate get_players(scores), to: Map, as: :keys
end

Feedback:

Recommended Use a default argument in the add_player/3 function to handle a lack of initial score. Use the module attribute with the default initial score.

bugQ avatar Mar 09 '23 01:03 bugQ

@bugQ Thanks, this should fix it: https://github.com/exercism/elixir-analyzer/pull/372

angelikatyborska avatar Mar 09 '23 07:03 angelikatyborska