which is preferable style: destructuring struct with pattern matching in function definition, or accessing struct members in function body?

foobar.ex

defmodule Foobar do
  defstruct :my_field, :another_field

  def do_a_thing_destructure(%Foobar{my_field: mf, another_field: af}) do
    something_else(mf, af)
  end

  def do_a_thing_struct_access(foobar = %Foobar{}) do
    something_else(foobar.my_field, foobar.another_field)
  end
end

So do_a_thing_destructure seems more explicit and feels like the "Elixir" way of doing it. However it quickly gets verbose and convoluted in practice, and imperative old style do_a_thing_struct_access ends up just being easier.

Is there an Elixir convention around this?

No idea about the Elixir convention. I prefer the second because it doesn't require assigning vars for each field in the struct, and is more explicit about those fields when they're referenced in the method call.

So this is a definite vote for #2.

The only golden rule, I've heard of on this is "If you can pattern match, do that."

And even though it looks like destructuring, pattern matching is essentially what's going on in the first function head. Aside from it cleaning things up, I'd also assume it's faster (all other things equal) given that it's native to the BEAM and not just Elixir.

I'm still new to Elixir, but I'd say that it's one of those things you have to make a judgement call on a case-by-case basis (no pun intended). Too many pattern-matched variants of a function, and you start adding unnecessary complexity (thinking back to the good ol' C++ operator/function overloading insanity) and also run the risk of building non-DRY code. Not to mention if I'm not mistaken (which I could be), you've got to make sure you order the pattern-matched functions correctly or they might not get invoked as you expect.

I think the Golden Rule, if any, is to make your code clean, legible, and maintainable regardless of which mechanism you choose.

I prefer #1. My reasons:

1. Explicitness: If your function depends only on :my_field and :another_field, I think it's much better to define those dependencies explicity in the function's signature.
   This way it's easier to reason about the behaviour of the function.
2. Easier to test: Since the dependencies are explicitly, you don't have to "parse" the body of the function looking for which fields you need to setup to get the test running
3. Compile time check: In #1, if the name of one of those fields changes, you get a compile time error right way. In #2, you'll only get an error in runtime, maybe in production.