Good and Bad Elixir

> I regret titling the post with the words "good" and "bad".

The title struck me as exactly what I would expect from you (based on various articles/comments and your podcast), and fwiw, I personally really appreciate it.

You're one of the people in the community that communicates in a way that often strikes me as 'overly' opinionated, but the combination of that and your actual experience tends to lead to fruitful discussion (see this thread). Plus, I often find myself wanting to 'prove you wrong' and then I end up doing a bunch of equally fruitful research.

So keep doing your thing, is what I'm saying, I suppose. It's appreciated :).

Thank you for writing it! It was a pleasure to read and I appreciate you contributing to the community.

I think it's important for teams to establish best practices and enforce them. It's good for team cohesion and culture.

For example I've worked on teams that blocked PRs on minor typos in code comments. The signal this sent to me was "a little goes a long way, we care about the little things".

Sure enough that ended up being the cultural norm and the team benefited from that mentality. There were also downsides though! That's part of the trade-off, but the benefits of a unified "culture" outweighed the downsides by far.

What's reeeaaally bad is when you have competing points of view between tech leads and they passively enforce their own idea of what's "right" without regard for the other lead.

Agreed, this was a huge pain in the past for me. Then, the even MORE senior engineers are just like "Ok do the tests work? Yes? Ok whatever, approve"

So there's this middle section of "senior" (but still in age terms relatively junior in their lifecycle) that are just trying to exert dominance over a dumb web app that controls the flow of the spice (errmm.. code) in the org. It's really silly and deeply counterproductive.

I agree with so much of this! I think it stems from so many software engineers have a quantitative background, where solving for x is either right or wrong. Much of “good” software engineering, however, is subjective and I think many of these folks have a difficult time reconciling that. So you get into these battles on which is the best way when, unless it’s something egregious, is probably good enough.

I have a writing and liberal arts background and so while bad writing is obvious, good writing comes in many stylistic forms. I think this has allowed me to see varying perspectives on “best practices.”

I've noticed this is much more pronounced if there's been no discussion before the PR (talking more about larger decisions than what's in the bulk of code review feedback I guess). I've observed many times people being frustrated because they write some code without making sure the relevant people are happy with the overall approach, then getting torn apart in the PR.

I do think that being opinionated about code style leads to code that's easier to read and maintain. I was just delighted to find someone whose tastes diverged so distinctly from my own in a smaller language community.

But how do you take this to a code review scenario? Are these suggestions that you can take or ignore?

I would recommend writing specs for your functions and using Dialyzer, too :)

I totally agree, though I think those articles are a lot harder (eg requiring more skill) to write well because you need to quickly ramp your readers on all of whatever the context is that's necessary to actually appreciate the nuance of the design decisions under discussion. You're basically by definition going to be out of the realm of "just follow best practice X" or "apply pattern Y or you're doing it wrong."

As a small example, I've been working on a small asyncio-based web service (Python) which is oriented around an expensive process that generates a result, where the result is stashed in sqlite and returned. I knew upfront that I needed a way to track when a particular result was already being prepared so that if I got a second request for it, it would collapse it into the first one and only do the work once. I wrote this as a twenty line memoizing decorator, but it turns out this issue as a name— cache stampeding. Once I realized that, I discovered that there are existing (and much more complicated/tunable) solutions to this problem, such as https://github.com/DreamLab/memoize/, but the article pitching that solution spends quite a bit of time getting to it— enough so that if I'd discovered it before building my own, I'm not sure I would even have appreciated its applicability:

https://tech.ringieraxelspringer.com/blog/open-source/cachin...

I dunno, I rarely get issues that crash just once. For example, in the blog post, it's recommended to use `Jason.decode!`. But what if whatever server you're hitting is returning invalid JSON for a time? If you crash, the GenServer will restart, it will crash again, and it will just be either in a crash loop (meanwhile DOS-ing the server?), or worse - if it happens in `init` - will take down its own supervisor after a number of failures, cascading up the tree until your app crashes and restarts.

I think it's better to handle the Jason.decode error, and apply some sort of backoff strategy on re-requests. In general, you have so little control over the timing and count and rate of restarts. It's a very blunt object to just crash. Again, with the `Jason.decode!` error, suppose it's in a GenServer that's updating its state. In some cases you may want to reset the state to empty (essentially what crashing does) and in other situations you may want to keep the previous state, from the last time you made that request. Maybe you want to track how many consecutive failures you're seeing and maintain the state for some time, but eventually blank it, or crash at that point.

It could very well depend on the problem domain, though, for sure. I mostly work on freestanding, pure Elixir apps, and a couple of Phoenix apps. I'm more thinking about the freestanding apps, since in Phoenix, sure, just crash your request, whatever.

In the spirit of Python's "Exceptions are when the runtime didn't know what to do, and Errors are when the programmer didn't know what to do", I'd say "rely on crashes for exceptions, but validate data against errors".

Reminds me of the following from my Ruby/Rails days: https://api.rubyonrails.org/classes/ActiveSupport/HashWithIn...

Elixir, like Erlang, is functional so there are no statements. Everything is an expression, which evaluates to some value when executed. The “return” value is just the value of the last expression evaluated in the function call.

Elixir does not have a return keyword.

The author uses the word "religion" in a jocular manner, which seems appropriate, but probably not in the way he intended. This particular style often seems to get applied as a dogmatic pattern. The underlying principles are good, but the author should have stopped halfway.

For example, the article has this:

  handle_db_create_msg(Msg, State) ->
      log_operation(db_create, Msg),
      handle_db_create_result(create_db(create_db_args(Msg)), Msg, State).

  create_db_args(Msg) ->
      [create_db_arg(Arg, Msg)
       || Arg <- [name, user, password, options]].
  
  create_db_arg(name, Msg) -> db_name(Msg);
  create_db_arg(user, Msg) -> db_user(Msg);
  create_db_arg(password, Msg) -> db_password(Msg);
  create_db_arg(options, Msg) -> db_create_options(Msg).
  
  create_db(Name, User, Password, Options) ->
      stax_mysql_controller:create_database(Name, User, Password, Options).

It would have been much better to stop at this point:

  handle_db_create_msg(Msg, State) ->
      log_operation(db_create, Msg),
      Name = db_name(Msg),
      User = db_user(Msg),
      Password = db_password(Msg),
      Options = db_create_options(Msg),
      Result = stax_mysql_controller:create_database(Name, User, Password, Options),
      handle_db_create_result(Result, Msg, State).

There are lots of contradicting advice in the Erlang/Elixir land.

Garret Smith likes microfunctions, while Saša Jurić avoids them.

Same with "tagged with statement" pattern - Saša Jurić prefers it over Ecto.Multi in transactions, while this post discourages it's use at all.

I notice he says the result needs less testing; to me such a rewrite amounts to an audit of the code and thus leaves one with more confidence, definitely. I'd just buy it more perhaps with stringent static types, but I've become a 'gradual typing purist' in a way (in this process, if you're not littering types everywhere you are missing a lot of value)

No. It's the same. If you have a function with a large if statement in it, you'd want to test all conditions within that if statement to make sure it was working, right?

You'd do the exact same thing if you were testing a series of pattern-matched functions. The tests would be unchanged either way.

I started my Elixir journey with Dave Thomas, too, and I agree with your commentary. I think most of Dave Thomas' style is highly organized and readable.

One of my favorite Dave Thomas tidbits was writing private functions like this:

  defp _private_function(params) do
    # Do something
  end

Readability and organization are so important in code and I felt that Dave Thomas helped me get off to a great start in this regard.

The more I need to handle errors, the more I want to make every unexpected thing just raise an exception. 500s for everyone!

Exactly, piping a bunch of ! functions is the way.

Uh I think the rule of thumb is, if you're writing a library do return an error tuple because it gives the caller the option, and if you're a mensch also provide a bang function. If you are consuming a library in most cases let it crash.

let it crash is great for unrecoverable errors as it gives you clean state as if the thing that caused the error never happened but it's terrible for cases where you want to actually capture the error and react to it. give me a structured log event or an error code every time over a stack trace

I didn't make a very strong case for it. Overall, my experience has been that calling the side-effecting function in the case statement directly tends to be easier to maintain and easier to read. But, I think piping into case can work well in certain situations. Its not one of the points that I feel that strongly about.

> The use case I had in mind was specifically how people will use `Keyword.get` to grab configuration values or options.

It seems like this advice is very dependent. I think you should decide whether a function accepts maps or keyword lists or both, but I do not think that all functions which operate on key-value structures should default to accepting both. They have very different performance characteristics, and keys are not unique in keyword lists.

> That said, I think this advice is particularly useful for people building libraries more so then people working on applications

This is a good point that I hadn't really thought about previously. I agree with your thoughts much more when I consider the different needs between libraries and applications.