Pattern matching in function declarations when not needed for multiple clauses - yes or no?

Yay.

The only problem I’ve faced with this approach so far are cryptic error messages on a failed match:

** (FunctionClauseError) no function clause matching in Test.run_before_scenario/2

With a failed match inside the function body the error messages are a bit less cryptic since they at least provide the unmatched value:

** (MatchError) no match of right hand side value: nil

Same here, I do the head-style thing. One benefit would be easier further clauses incl. catch all that’d do some logging to avoid the error messages problem @idi527 mentioned.

If it’s not required for selecting the right function clause I tend to keep the pattern matching simple. Instead of matching on the whole structure just use %Scenario{} = scenario. In case I need to match more than a single key, I would put that in the function body.

I think this is great, but Typespecs serve the same purpose and are a better fit if this is what you’re looking for. Even without using dialyzer typespecs offer the benefit of better documentation and serve as a tool to clarify your thinking on what the function is supposed to do.

Imo they don’t serve the same thing - a typespec is "this takes a Scenario.t and a ScenarioContext.t (which both have like ~10 keys) returning … - which is great in its own right and I use typespecs with dialyzer a bunch. However, it isn’t “I use before_scenario and input from the first argument and … from the second argument”. Granted, it’s not too much of a difference while working with structs instead of plain maps. It can help for instance for test setups or other stuff though.

Of course I guess you could do the same thing if you defined a new type if you just care about specific keys but that’d be weird.

I think heavy struct pattern matching is a sign that the function is getting passed too much stuff through its arguments. James Hague puts it like this:

I’d go so far as to say that if you pass a record-like data structure to a function and any of the elements in that structure aren’t being used, then you should be operating on a simpler set of values and not a data structure. Keep the data flow obvious.

The easiest way you can make it obvious that the function only uses this and that is to pass them directly as arguments, without using a struct:

    run_before_scenario(input, scenario.before_scenario, scenario_context.config.before_scenario)

    def run_before_scenario(input, before_scenario, global_before_scenario)
      ...
    end

It’s not immediately pretty, but it makes it easier to see what the function is about: it just takes some
kind of callbacks and applies the non-nil ones. We can generalize it to make that clearer and reduce the clutter, something like:

    callbacks = [scenario_context.config.before_scenario, scenario.before_scenario]
    scenario_input = run_optional_callbacks(scenario.input, callbacks)

    def run_optional_callbacks(input, callbacks)
      callbacks
      |> Enum.reject(&is_nil/1)
      |> Enum.reduce(input, fn (callback, input) -> callback.(input) end)
    end

This removes the “deep pattern matching in function head” problem, and the helper function becomes a lot easier to unit test, since it only takes generic Erlang types as arguments. It’s also easier to change the struct when it’s not weaved through all layers of the application.

This is a very good point. The great diffing of FunctionClauseErrors showing the function heads and what did and didn’t match is a very nice thing to have. I didn’t consider that in my support for the Nay camp.

I usually go this route because then I know when I’m seeing pattern matching in a function head, that the pattern matching is necessary for that function to work correctly, and so I can’t take it out of the function head or else I’m changing behavior. And also because that’s what they do in elixir-lang core, and I like to try and stay consistent there to try and encourage a single community style, which I probably think is more important than it actually is.

The great diffing of FunctionClauseErrors showing the function heads and what did and didn’t match is a very nice thing to have.

I think it works only in dev since it’s expensive to compute. I don’t see these diffs in my prod logs. Unless that’s what you meant and I misunderstood you …

That guideline make sense when your function has multiple clauses. The idea being that if you are using pattern matching as a discriminator, do as little matching in the function head as necessary because any additional matching effort will be lost if that particular function clause does not execute.

The guideline doesn’t really apply to single clause functions - the only real difference being where the match failure will occur; inside the function or during function clause match.

If you are destructuring (e.g. set nil if no match) then you will either need another function clause or a case inside the function.

That is an interesting thought and I can certainly be guilty of passing structs too much. It’s a good thought that this might be a smell for overusing them.

It moves the knowledge of where the callbacks reside and what arguments are needed out of the function itself and to its caller - which can be a boon, but here it’s not imo

The function is only public because it has 2 callers - which would spread the knowledge of the concrecte “structure” of the struct further and as a result make it harder to change than when it resides in this module.

I agree you should definitely be careful in moving internal knowledge out of a function to its caller. I don’t agree with James Hague here as it means you are forcing knowledge of the internals of the data structure out where it is probably not supposed to be visible. If you were to follow this rule for every record-like data structure then you would have to know about their internals everywhere. And yes, this is pushing it ad absurdum but you get the idea.