In some cases I wouldn’t care about FunctionClauseError and my error reporting system in the project would still catch it and I’d know almost immediately what the problem was. Still, by doing this refinement I believe we can invest in more long-term readable / maintainable code. But again, depends on scale / size. For smaller code sizes it’s hardly worth it.
So maybe not exactly subjective, more like project-specific I’d say.
Basically: use function-head pattern-matching to select which function to enter, then do additional destructuring inside of the selected function. That policy I fully agree with.
But patterns that are irrelevant to flow control can easily be rewritten to become relevant. An example:
def same_age(%Person{age: age1} = person1, %Person{age: age2} = person2) when age1 == age2 do
if is_nil(person1.name) || is_nil(person2.name) do
"invalid names"
else
"#{person1.name} and #{person2.name} have the same age"
end
end
def same_age(%Person{} = person1, %Person{} = person2) do
if is_nil(person1.name) || is_nil(person2.name) do
"invalid names"
else
"#{person1.name} and #{person2.name} have different ages"
end
end
The person’s name is not used for flow control, so it’s not destructured in the function head. That’s good. However, if I want to make it part of flow control, I can do it:
def same_age(%Person{name: nil}, _) do
"invalid names"
end
def same_age(_, %Person{name: nil}) do
"invalid names"
end
def same_age(%Person{name: name1, age: age1}, %Person{name: name2, age: age2}) when age1 == age2 do
"#{name1} and #{name2} have the same age"
end
def same_age(%Person{name: name1}, %Person{name: name2}) do
"#{name1} and #{nam2} have different ages"
end
Now the person’s name is suddenly part of flow control, so it’s ok to destructure it in the head?
Flow-control ceases to be arbitrary only when you have a clear idea of what function clauses you want and what they’re doing. I guess after that one is fixed, you can make the distinction between patterns used for flow control and patterns used otherwise.
I don’t understand this line of thinking. Are you saying people shouldn’t bother writing readable/maintainable code in small codebases? Wouldn’t you just write in the “good” style at all times, for the sake of habit, courtesy, etc?
I am saying that return-of-investment for coding the separation of flow control and destructuring in particular might not be worth it. You should still employ every best practice that you know, even in small codebases, for as much as your time budget allows you.
It’s just that this particular practice doesn’t seem worthwhile to pursue until you stumble upon a situation where not following it bites you gently on the arse. When that happens, you then roll up your sleeves and make it more maintainable.
I was not saying it should be 0% maintainable to start with, no.
I remain to be convinced that strict separation of flow control matches and the other matches improves readability and/or maintainability. I’m not saying it doesn’t, just not convinced yet. I suspect it’s true however since all you guys seem to agree on this one
I’ll try to consciously apply this separation when I write code now, and see where it takes me.
Same here, I am 50/50 about it but the examples given so far have convinced me that it can be genuinely useful in certain situations so that’s at least worth keeping in mind (as you alluded to).
The separation becomes useful when pattern matching just to extract values is abused. I’ve seen A LOT of code like this:
def send_to_manufacture(%Order{
id: id,
customer: customer,
price: price,
discounts: discounts,
processed: true,
items: items,
due_date: due_date,
assigned_facility: facility
...
} = order, opts \\ []) do
...
{:ok, order}
end
def send_to_manufacture(_, _) do
{:error, "Some explanation of the error"}
end
Can you tell at a glance what is actually deciding which clause will be executed?
Why not just use strict access in the body instead:
def send_to_manufacture(%Order{processed: true} = order) do
shipping_cost = Shipping.calculate_cost(order.customer.address, order.assigned_facility.address, order.items)
Manufacture.notify_facility(order, order.assignted_manufacturer)
Manufacture.notify_customer(order, order.customer)
...
end
def send_to_manufacture(_, _) do
{:error, "Some explanation of the error"}
end
It applies to both maps and structs, though structs already give you the guarantees that the keys exist so you just focus on the values you want.
The strict access syntax will raise with a ** (KeyError) key :foo not found in: %{bar: 42}.
Bonus points if send_to_manufacture/2 has proper documentation but I don’t find pattern matching to be particularly easier to mentally parse when you’re extracting way too much. I’ve seen very long matches where you don’t know where the heck is this being extracted from until you find that tiny little } = order at the end.
Yep, totally convinced now. Time to confess my sins: I’ve written quite a bit of code like this myself. Thought it looked “cool” and “elixirly” when I was writing it, now I realize it’s just pretty unreadable when you want to understand which clause will be executed. It will be fun to refactor it. Thanks for the nice example.
