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.

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

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.