Suggestion of smell: mixing extraction and conditionals in pattern matching

[josevalim]

Take the following code:

def drive(%User{name: name, age: age}) when age >= 18 do
  "#{name} can drive"
end

def drive(%User{name: name, age: age}) when age < 18 do
  "#{name} cannot drive"
end

In the code above, the pattern matching of the user is extracting fields for further usage (name) and for pattern/guard checking (age). While the example above is fine, once you have too many clauses or too many arguments, it becomes hard to know which parts are used for pattern/guards and what is used only inside the body. A suggestion is to extract only pattern/guard related variables in the signature once you have many arguments or multiple clauses:

def drive(%User{age: age} = user) when age >= 18 do
  %User{name: name} = user
  "#{name} can drive"
end

def drive(%User{age: age} = user) when age < 18 do
  %User{name: name} = user
  "#{name} cannot drive"
end

This is also related to "Complex multi-clause function".

[fishtreesugar]

I am missing "non-exhaustive pattern" checking in language like Haskell. We're very easily create a partial function when conditions more complicated or more and more clauses.

or How to "make illegal states unrepresentable" in Elixir?

[josevalim]

It think that’s a separate discussion as it is more about language features and this one would require a type system and even then exhaustiveness checks may not be that straightforward. In the lack of static types, you make illegal states unrepresentable by not representing them. :)

[lucasvegi]

Great suggestion @josevalim!

It is really related to "Complex multi-clause function", but with implications of its own. I'll add it to the catalog.

I thought of using a shorter and more objective name, like "Complex extraction in function signature". What do you think about him?

[lucasvegi]

Maybe "Complex extraction in signature" to get even smaller... :)

[josevalim]

Either that or "Complex extraction in clauses", since it may apply to case, receive, etc. :)

[lucasvegi]

Great! "Complex extraction in clauses" sounds good!

[lucasvegi]

I just added this smell to the catalog. What do you think?

https://github.com/lucasvegi/Elixir-Code-Smells#complex-extraction-in-clauses

[tiagoefmoraes]

What do you think of leaving the Struct out of the extraction?

def drive(%User{age: age} = user) when age >= 18 do
-  %User{name: name} = user
+  %{name: name} = user
  "#{name} can drive"
end

[josevalim]

Looks great, both ways are fine by me! :)

[lucasvegi]

Hi Tiago,

Thanks for your comment. Your way is valid too. But I think the ideal would be to standardize the extraction forms (in the function signature and internally). I particularly think that the extraction using the struct is more in line with the smell description, so I prefer to leave it as is. :)

Do you think we can close this issue (@josevalim and @tiagoefmoraes)?

[AdamT]

Hi there,

I know this is closed but just had a question about why a user name is being extracted when we can avoid most of the typing by calling dot .:

Before

def drive(%User{age: age} = user) when age >= 18 do
  %User{name: name} = user
  "#{name} can drive"
end

def drive(%User{age: age} = user) when age < 18 do
  %User{name: name} = user
  "#{name} cannot drive"
end

After:

def drive(%User{age: age} = user) when age >= 18 do
  "#{user.name} can drive"
end

def drive(%User{age: age} = user) when age < 18 do
  "#{user.name} cannot drive"
end

By removing the extraction in the method body we make the code "easier" (maybe?) to read. That also means the complexity of the function is where most of the code is, in this example it's in the function definition which means users will focus more attention on the function definition when skimming. Lastly, we know that a user has been pattern matched in, so it is safe to know we can call user.name. Just because we were previously able to pattern match a value out of a struct on the function definition doesn't mean we have to use it in the body. Once moved into the body we can re-evaluate the best way to get the name.

Maybe this is another code smell discussion 😅 "Don't use extraction when a dot will suffice" 😄

(Thanks for all the work btw!!!)

[josevalim]

Good point! The dot and pattern matching are semantically equivalent in this case. You could use the dot, but if you prefer to use dot, then it is less likely you will run into this issue anyway. :D so I would personally keep it as is!

[lucasvegi]

Excellent observation @AdamT!

I will even think about the suggestion of "Don't use extraction when a dot will suffice".

Anyway, about the current smell (Complex extraction in clauses), I think @josevalim comment represents what I think too:

You could use the dot, but if you prefer to use dot, then it is less likely you will run into this issue anyway

The idea behind this smell is not necessarily to say that pattern matching/extraction is a bad practice and should not be used. I think it's more along the lines of: "If you're going to use this feature, it's better to use it this way..." 😃