Credo.Check.Refactor.MatchInCondition explained

[mgwidmann]

A brand new phoenix project triggers this warning, and when I went to explain it all it says is that it shouldn't be done. Can you explain why this is considered bad form or why it should be avoided?

Heres the function phoenix generates in the error helper:

  def error_tag(form, field) do
    if error = form.errors[field] do
      content_tag :span, translate_error(error), class: "help-block"
    end
  end

[rrrene]

I should really improve the explain text for that check. 😕

The reasoning is that pattern matching in a condition will make the code harder to reason about, especially when maintained by a different person than the original author. Assigning in a condition can also be mistaken for a comparison (e.g. when working under deadline pressure).

Example:

  if {:ok, %{"error" => true, "message" => message}} = parameter1 do
    do_error_handling
  end

In more complicated cases like this, the code might be more maintainable in the future if it reads like this:

  case parameter1 do
    {:ok, %{"error" => true, "message" => message}} -> do_error_handling
    _ -> nil
  end

I am guilty of writing code like this in the past, as I thought it to be clever and less verbose than the alternative. But the path to hell is paved with clever solutions and good intentions.

That said, nothing Credo suggests is set in stone. Maybe we should have a configuration option to allow simple assignments like the one in your example?

[mgwidmann]

I believe your example is no longer a conditional. Meaning, if the match fails it won't execute the code either way, thats certainly worth warning about.

However, in the most common use case of the match operator is actually a presence check.

if parameter = get_parameter_data() do
  do_something_with(parameter)
end

Which is just shorthand for

parameter = get_parameter_data()
if parameter do
  do_something_with(parameter)
end

In my experience, this is ok, as long as the match is entirely a wildcard (only a variable). I believe this error message should only trigger for "complex" matches, basically anything except a variable inside a conditional.

[mgwidmann]

Wanted to point out this is done in the Elixir codebase: https://github.com/elixir-lang/elixir/blob/574c6db3f8566a941b55c66499078d61661d08c6/lib/elixir/lib/kernel/cli.ex#L437-L448 and I'm sure in other places as well.

I would vote for a warning about any "complex" (or non-wildcard) type matches. Simply capturing the return value shouldn't be a problem unless it's the same name as another local variable/parameter (since that would then be bad if it changed to a ==)

[rrrene]

I heard you 😉 The behaviour will change like we discussed above.

Development is a bit slow at the moment, because there is a lot of stuff happening at my day job AND I am giving a talk about Credo at next week's ElixirConf.EU in Berlin and the preparation is taking up most of my free time. 😁

[philipgiuliani]

Awesome! Just looked for the same thing. Its possible to use 0.4.0 already now?

PS: Your talk was amazing @rrrene ! That's why im using Credo now... :)

[rrrene]

I will release a beta this evening. Still working on the "create your own checks" UI, but most of the other stuff is done.

Will ping you here when it is released! 👍

[rrrene]

@philipgiuliani @mgwidmann Sorry, I forgot to ping you. This is part of v0.4.0-beta1.