What is the preferred map pattern-matching style in a function parameter?

I know the title is a mouthful. Here’s an example

# pattern-to-match = param
def a(%{name: name} = person), do: {name, person}

# param = pattern-to-match
def b(person = %{name: name}), do: {name, person}

I generally see the pattern matching done in this order:

# pattern-to-match = param
def a(%{name: name} = person), do: {name, person}

I know they both yield the same result. Does anyone prefer one style over the other? Should the formatter handle this? Much respect.

2 Likes

Hello and welcome,

You might find an answer here…

I always use the form %{} = person, but You can use both :slight_smile:

2 Likes
def b(person = %{name: name}), do: {name, person}

Why would s.o. want to do this?
Why is this even allowed?
Outside of a def header it is not:.

> foo = %{a: 1, b: 2}  
%{a: 1, b: 2}

> foo = %{a: 1, b: bar}
** (CompileError) iex:8: undefined function bar/0 (there is no such import)

> %{a: 1, b: bar} = foo
%{a: 1, b: 2}

Because a = %{a: b} also can be a pattern:

a = %{a: b} = %{a: 1, b: 2}
2 Likes

makes sense, so we actually are only writing the left side of the pattern in the def header.

def b(person = %{name: name} = incoming-argument-from-caller)

So one could argue that
def b(person = %{name: name})
is actually the most consistent way.

EDIT: nonsense.

Why that would be more consistent? You can order them in any way you want as = operator returns RHS. I find the ordering more_specific = less_specific = source more appealing as it visually works like sieve. You first match on less specific pattern and then on more specific pattern “sieving” the source through patterns.

9 Likes

right, I was confused.

AFAIK, credo generates warning if the style is inconsistent across the codebase.
https://hexdocs.pm/credo/Credo.Check.Consistency.ParameterPatternMatching.html

2 Likes

The first way seems to be the prevalent way in Elixir code. The second way seems to be the prevalent way in Erlang code.

1 Like

just to confirm. just visually. There is not way to change what’s bound in a match by reordering, right?

I have not heard of a community/language/credo preference.

It is probably more important that you pick one style with your team and stick with that everywhere, than which one you pick.

At work we chose:

def a(%{name: name} = person), do: {name, person}

I think this especially benefits people coming from other languages where they have no experience with pattern matching. This form makes it clear that this is not an assignment operator doing assignment.

3 Likes

I’ve been in a team where they did NOT prefer this style:

def do_stuff_with(%{field_a: a, field_b: b}) do ...

Because, they said, if you call the function with unexpected argument shape, “you’d get a cryptic FunctionClauseError that does not help you track down where the error is” – I don’t remember well.

They were very focused on having clear(-er?) stack traces. And they preferred the following style:

def do_stuff_with(arg) do
  %{field_a: a, field_b: b} = arg
end

They said this gives a better stack trace because it’s from inside the function.


In the end I complied with their style but I remained unconvinced; after all, a FunctionClauseError does give you a location from the offending caller. But maybe the argument was that it’s not showing the function argument that was given? I don’t remember now, wish I wrote it down.

They had a good point, admittedly, but I still didn’t like the resulting code. Felt like it’s not fully using Elixir to its strengths somehow.

1 Like

When the match becomes too complex, it can be hard to distinguish between matches that are there for flow control and those that are only there to extract values. So for complex functions, I tend to only match for control flow things in the header and then extract them in the body.

For example in a function where I know that b will always exist, I would do this (excuse the Erlang, my Elixir is terrible):

-spec foo(#{ a => atom(), b := atom()) -> atom() | map().
foo(#{ a := A } = Map) ->
   #{ b := B } = Map,
   B;
foo(Map) ->
  Map.

instead of this:

-spec foo(#{ a => atom(), b := atom()) -> atom() | map().
foo(#{ a := A, b := B } = Map) ->
   B;
foo(Map) ->
  Map.

It then becomes clear what matches are needed for flow control and which are not. This becomes more and more important as the number of clauses grows in a function. Using records (or structs I assume) gets around this problem, but even there it can become hairy if you match the shape of a specific value in a record.

8 Likes

(EDIT: Entirely replaced the previous comment since I misunderstood your post.)

I see. Nice distinction between flow control and extracting values.

Really drives the point home after it’s presented like that. Thank you.

2 Likes

Great explanation…this is one of the Elixir code smells called Complex extractions in clauses:

I heard this argument a few times already and I have to say I find it hard to grasp the distinction between “flow control” and “only there to extract values”. One could argue that your function:

def foo(%{a: A} = m) do 
  %{b: B} = m
  B
end

can’t be successfully executed without m having :b as a key, so why can’t :b also be seen as part of flow control?

For me it is a maintainability question. As the program evolves and the relation between :a and :b changes, it becomes clearer what changes can be made without breaking existing code. And if I do change something, the crash will happen a lot closer to the problem in the code.

In small code examples I don’t find that it matters all that much, but as the code grows into huge complex functions I find that it becomes easier to understand the code I wrote a couple of years (or days…) ago if I structure it like that.

An example of a place where I probably should have done it this way but didn’t is here in user_drv:server/3.

server(info, {WriteRef, ok}, State = #state{ write = WriteRef,
                                             queue = {{Origin, MonitorRef, Reply}, IOQ} }) ->
....
server(_, _, _) ->
    keep_state_and_data.

This gen_statem has to throw away any messages it does not know how to handle. So if something does not match any of the clauses it is just thrown away. In the specific clause I’ve quoted I know that if I get that type of message with WriteRef there will always be a queue present. So if there isn’t a queue something is very wrong. By writing the code like this:

server(info, {WriteRef, ok}, State = #state{ write = WriteRef, queue = Q }) ->
    {{Origin, MonitorRef, Reply}, IOQ} = Q,

I will get a crash in the correct location if there ever such an event, instead of the message just being silently dropped.

4 Likes

Thanks. I feel things get more interesting when you’re dealing with more realistic examples.

I feel that in Elixir at least these cases can be handled with structs:

def server(info, something, %State{
        write: write,
        queue: %Queue{
          origin: origin,
          monitorref: monitorref,
          reply: reply
        }
      }) do
  do_something_with(origin, monitorref, reply)
end

def server(_, _, _) do 
  :nothing
end

now there’s no way I can call server with a queue which is not a %Queue{} and hit the catchall clause. Again my question, is queue part of flow-control? In my opinion it is.

But then I can see that rewriting it like this:

def server(info, something, %State{
        write: write,
        queue: queue
      }) do
    %Queue{
      origin: origin,
      monitorref: monitorref,
      reply: reply
    } = queue

    do_something_with(origin, monitorref, reply)
end

It’s telling me more clearly: “I just expect a %State{} with write and queue”. So we made the “flow-control” part less specific and less verbose I guess. I can live with this version of the code.

But then one could say: “hey, being a struct, %State{} will always have write and queue that’s the point of being a struct. So matching those fields is not “flow-control”, it’s variable extraction”. Let’s rewrite:

def server(info, something, %State{} = state) do
    %State{queue: queue} = state

    %Queue{
      origin: origin,
      monitorref: monitorref,
      reply: reply
    } = queue

    do_something_with(origin, monitorref, reply)
end

Ok, now I guess the flow control part is reduced to a minimum. But I confess I don’t like this version, I liked the first one the most because I could clearly see what the function was expecting and using just by looking at the head.

All this to say that I find a lot of these observations useful but also subjective, and the distinction between flow-control and something else is a bit arbitrary IMHO. I guess you could argue that stripping the head of matches improves readability, but that’s also subjective: I personally find the first version to be the most readable, if properly formatted as I posted it.

1 Like

Oopp! I was wrong here :grin: . You would need to add another clause to catch this. So I guess the second version is probably the one I’d prefer… But only because of this catchall clause, otherwise I’d prefer the first one

Flow control is about deciding which function head to execute, when there are multiple ones. If that’s done using patterns it can be rather useful to not mix those up with patterns, which are irrelevant to flow control, but only used to extract some values out of their containing parent. That may or may not fail within the chosen function body, but it at least doesn’t fail in the function body selection, but is clearly scoped to just the one function body selected.

3 Likes