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

Title basically says it all - to be more precise: When writing function do you already pattern match on everything you need in the function head even if it is not relevant to selecting multiple clauses?

Afaik the rule in elixir-core is to only match on things that are needed to select the correct clause and other matching is done in the function body.

To illustrate a bit here is an example from benchee and the alternative option courtesy of @devonestes:

  # match everything in the head
  def run_before_scenario(
        %Scenario{
          before_scenario: local_before_scenario,
          input: input
        },
        %ScenarioContext{
          config: %{before_scenario: global_before_scenario}
        }
      ) do
    input
    |> run_before_function(global_before_scenario)
    |> run_before_function(local_before_scenario)
end

or:

  # match in body
  def run_before_scenario(scenario, scenario_context) do
    %Scenario{
      before_scenario: local_before_scenario,
      input: input
    } = scenario

    %ScenarioContext{
      config: %{before_scenario: global_before_scenario}
    } = scenario_context

    input
    |> run_before_function(global_before_scenario)
    |> run_before_function(local_before_scenario)
  end

Admittedly long pattern matches in the function definition like above are very unusual starting out but they’ve grown on me. I see them almost as a “structual” type definition of my function “this function needs 2 maps with the following keys” and I really like that.

The second version almost has the same benefits just more in the body and a much cleaner/non multi line function head. Also as this is what is done in elixir-core I should probably trust them :smiley:

So, what’s everyone doing - multiline function heads without clause match need, Yay! or Nay!

1 Like

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
3 Likes

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.

2 Likes

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.

1 Like

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.

1 Like

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.

3 Likes

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.

1 Like

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.

1 Like

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 :slight_smile:

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.

8 Likes