exercism / elixir-analyzer

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

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

Closed TBK145 closed 1 year ago

TBK145 commented 2 years ago

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.

jiegillet commented 2 years ago

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.

angelikatyborska commented 2 years ago

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.

bugQ commented 1 year ago

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

angelikatyborska commented 1 year ago

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

bugQ commented 1 year ago

@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.

angelikatyborska commented 1 year ago

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