Universal checks for every single solution?

[angelikatyborska]

As I mentor, I keep seeing some reoccurring problems with solutions that are exercise-independent and could, at least partially, be detected automatically.

A code snippet from a real recent solution:

@spec reverse(list) :: list
defp reverseHelper([], reversed) do
  reversed
end
defp reverseHelper([x|xs], reversed) do
  reverseHelper(xs, [x|reversed])
end

def reverse(l) do
  reverseHelper(l,[])
end

The analyzer could detect and give a suggestion to the user that:

• @spec for a function should be placed directly above that function definition, there shouldn't be a different function definition in between
• function (and variable) names should use snake_case

More opinionated, but we could also check if each public function has a @spec and @doc and nudge students to add them (for practice exercises only).

Without parsing the AST, we could check if the code is intended with spaces or with tabs.

I think in particular the camelCase and indenting are worth doing in the analyzer, because they're hard no-nos in community Elixir code, but they're annoying details that are pretty uncomfortable to point out to a new comer. It would be nicer if a machine did that 🙂.

[neenjaw]

I like this idea in moderation. The casing stuff I think is important enough to warrant extensions. The indenting, I would rather point people to mix format rather than wade into a discussion of tabs vs spaces.

[neenjaw]

We could create a mix format microservice (picoservice?) which accepts a chunk of code and "formats it" and returns it to the editor :P

[angelikatyborska]

The indenting, I would rather point people to mix format

In which way would you point people? From the context I'm guessing NOT by having an analyzer comment? Would you expect mentors to tell students that?

We could create a mix format microservice (picoservice?) which accepts a chunk of code and "formats it" and returns it to the editor :P

That would be great because students in the web editor otherwise won't have access to the auto formatting :D but I have a hunch it wouldn't be part of v3.0...

Now that the base work for having common checks is in there, I'm going to create separate tickets for separate checks so that other people can work on them too.

[angelikatyborska]

Added three so far that I was personally sure about. Feel free to add more or dispute specific ones 🙂

#103
#104
#105

[jiegillet]

How about adding checks for IO.inspect and IO.puts?

[neenjaw]

How about adding checks for IO.inspect and IO.puts?

If this were to be implemented, I would suggest to it be informational at most since there might be a specific reason they left it in. (Partial optimized solution in order to elicit/engage mentor feedback, etc)

[jiegillet]

Good point. I once had a weird process-related bug that depended on a IO.puts("") to work, I still submitted that to ask a mentor.

[angelikatyborska]

@jiegillet I would be fine with an informational comment telling the student to try to remove usages of IO.inspect and IO.puts after they finished debugging. It is a common problem. But you would need first to add a mechanism for specific "common checks" to be excluded from specific exercises. There are two concept exercises, for io and for file, that require using those two functions:

https://github.com/exercism/elixir/blob/main/exercises/concept/rpg-character-sheet/.meta/exemplar.ex
https://github.com/exercism/elixir/blob/1bef66fc55381ea44d02df36a7cef855e780a93b/exercises/concept/newsletter/.meta/exemplar.ex

[jiegillet]

How about adding informational checks about using @spec for non-private functions? And maybe @moduledoc? This may be too preachy :)
EDIT: I'm an idiot, those already exist.

[angelikatyborska]

There's a check for two-fer for those but not much else. Using @spec, @moduledoc and and @doc is a personal choice in my opinion. It's definitely helpful in bigger projects, but still optional. I wouldn't bother people with those in the analyzer. Especially because using them is a concept exercise on its own which means we cannot "require" people to know about them before doing that exercise.

[jiegillet]

We could create a mix format microservice (picoservice?) which accepts a chunk of code and "formats it" and returns it to the editor :P

That would be great because students in the web editor otherwise won't have access to the auto formatting :D but I have a hunch it wouldn't be part of v3.0...

Re-reading the thread, I think the idea was to add a common check that runs mix format on the student's code, and if there is a difference, dump the formatted code in an informational comment with "We noticed that your code didn't use mix format, a standard formatting tool in the community, here is what the formatted code would look like: ....". We could even have a celebratory comment that says "Your code is formatted perfectly, well done." is the code is well formatted.

[angelikatyborska]

I felt comfortable telling everyone about mix format when I knew that everyone is using the CLI (and only if their solution was horribly misformatted). Now with the web editor, getting a comment about a formatting problem, even if it's really tiny, might be annoying and hard to fix. The value of mix format is that you don't have to think about formatting, it just happens automatically. That analyzer comment would be the opposite of "not have to think about".

Could there be a way of only showing this comment if the formatting differences are... "big enough" (very specific term, I know :D)?

[jiegillet]

You're right, it made sense for the CLI, not for the web editor.
Quantifying differences... Mmh... Maybe it can be done? Maybe something like if the diff if more than 15-20% of the total character count?

Another idea I had was coming back to the compiler warnings. I revisited an older solution that a mentor commented on because I had a compiler warning. Now there aren't any ways to get to those warnings on the web editor. If there was a way to get compiler warnings in the analyzer, we could show :informational messages.

[jiegillet]

I thought of a new check. Quite a few times when mentoring I saw code like this:

cond do
  something? -> :a
  true -> :b
end

I think we could suggest to use if instead. What do you think?

I have another one, but it' more of a personal preference. Sometimes I see code with arguments piped into a single function like this argument |> function() instead of the simpler function(argument) and I wonder if it's not hurting readability, because when I see a pipe, I mentally prepare myself to follow the procedure and I feel shocked when it's over before it starts. As I'm writing this, I feel it's very nit-picky. Don't mind too much ^^

[angelikatyborska]

IMO both are too subjective for me to feel comfortable adding them to the analyzer. I agree with you that most two-clause cases should be ifs instead but not everyone feels the same. I have been in a casual fight about this with a bunch of devs over a beer 😂.

Something different that might be more objective is the if true do true pattern:

x = if a || b, do: true, else: false
# vs
x = a || b

[jiegillet]

OMG yes, that's just plain wrong :)

[neenjaw]

I thought of a new check. Quite a few times when mentoring I saw code like this:

cond do
  something? -> :a
  true -> :b
end

I think we could suggest to use if instead. What do you think?

I would disagree with adding this check, it assumes that something? is a single term expression and the result is a single value. I also think this is a matter of style where there is no canonical voice (that I am aware of) advising it.

[neenjaw]

x = if a || b, do: true, else: false
# vs
x = a || b

If a and b aren't both boolean values, then this isn't as silly as it seems.

I think the contrived example is

x = if a or b, do: true, else: false

[jiegillet]

How about a check that suggests Enum.map_join instead of Enum.map(list, &something/1) |> Enum.join("\n")?

[angelikatyborska]

Too subjective IMO, and yes, I say that mostly because I never use map_join 😇. I don't even know why.

[michallepicki]

An idea for a global check came up: when there's only one function clause with a body, and student additionally specified a "top" function head (e.g. to add a default argument), suggest that they can merge it with the only function clause. E.g.

  def add_player(scores, name, score \\ @initial_score)
  def add_player(scores, name, score) do
    Map.put(scores, name, score)
  end

could be

  def add_player(scores, name, score \\ @initial_score) do
    Map.put(scores, name, score)
  end

Or there could be a macro to specify checks that accept both formats, like in https://github.com/exercism/elixir-analyzer/pull/234/files