What are sort of smells do you tend to find in Elixir code?

Chat / Discussions
, ,
1

I’ll be deploying my first non-hobby Elixir application in the next few weeks, and I’ll be honest - some of it smells.

For those of you who’ve spent more time with the language, what are some code smells you’ve picked up over the years? What sort of techniques or approaches have you used to refactor those smelly concoctions?

Make it work, make it right, make it fast.
– Kent Beck

I feel like I can make it “work”, but in your opinion what does that workflow of making it “right” look like?

6 Likes
2

This discussion will probably be more helpful for you if you specify what do you find to be a code smell in your first professional Elixir app?

1 Like
3

I guess a general smelly area is when to use something like an {:ok, result} pattern for error handling. It seems unclear when or where I should handle different error scenarios. I’m kind of just building the happy path and handling what turns out to be fragile.

Something like the following makes sense for an API client, but I’ve got a few scenarios where I’m doing some JSON -> JSON transformations through some schema validations and what-not and I can’t help but feel there are some scenarios should be explicitly handling certain errors.

def get_customer(customer_id) do
  case get("/customers/#{customer_id}") do
    {:ok, %Tesla.Env{status: 200, body: body}} -> {:ok, body}
    {:ok, %Tesla.Env{body: body}} -> {:error, body}
    {:error, _} = other -> other
  end
end

Maybe this is just an imperative / defensive programming habit feeding me doubts.

1 Like
4

That is entirely up to your business requirements and what makes sense there. Let me rewrite your function:

def get_customer(customer_id) when is_integer(customer_id) do
  "/customers/" <> customer_id
  |> get()
  |> handle_3rd_party_api_response()
end

defp handle_3rd_party_api_response({:ok, %Tesla.Env{status: 200, body: body}}), do: {:ok, body}
defp handle_3rd_party_api_response({:ok, %Tesla.Env{body: body}}), do: {:error, body}
defp handle_3rd_party_api_response({:error, _} = other), do: other

Couple of points here:

  1. Breaking down local functions is very cheap. The compiler will freely inline things it feels belong together; it might internally merge get_customer with handle_3rd_party_api_response so it looks like your original function BUT your code is more readable and the more complex internal representation is not your concern.

  2. You can enforce all primitive types in function guards (notice the when is_integer(customer_id) in the function head).

  3. Please note that passing around ok / error tuples should not last forever, obviously. However they help immensely in managing your internal flow of logic and actions. In my apps these tuples eventually get fed to the serializers that are supposed to send responses back to the client caller (API consumer or a browser) and from then on the responsibility on how to process a success or an error lies with them.

You say:

Do not do that. Every function that can return an error tuple, you handle that somewhere. No exceptions. We all know how prototypes end up as being 10-year old mastodons full of buggy legacy code, right?

There is an Elixir library for JSON schema validation and I can link it if you like, however it’s my observation that most people only need mandatory keys enforcement and type checking of a few values. Again, you can use pattern matching in functions heads:

defp validate_3rd_party_api_response({:ok, %{data: %{records: records, endpoint: url}, page: page, count: count}} = response) when is_list(records), do: response

…etc. You can deconstruct to infinity and enforce a contract right when calling your function that is supposed to consume externally provided data. If any part of the structure is not okay, this should [again] be caught by your integration tests pretty early.

Does that help? I’ll be happy to address additional concerns. Just say what you are uncomfortable with.

6 Likes
5

This is very helpful - the intention was more in an abstract discussion on elixir code smells, but this is probably a better approach.

I can definitely see where an approach like

would make sense. This is maybe not the best example since here the case format is shorter and still very readable, but if there were significantly more cases and handling them were more complex, it would definitely make sense to use functions and parameter matching instead.

One area that I sometimes vacillate on is situations like this:

def map_builder(params) do
  %{}
  |> Map.put(:key, params["key"])
  |> Map.put(:key2, params["key2"])
  |> Map.put(:key3, params["something"]["nested_key"])
end

# or

def map_builder(%{"key" => value, "key2" => value2, "something" => %{"nested_key" => nested_value}}) do
  %{
    key: value,
    key2: value2,
    key3: nested_value,
  }
end

When would you prefer one or the other, or decide to switch? It feels like the pipe syntax works better in a higher abstraction and the latter at a smaller focus?

1 Like
6

I come from C and Assembler (17-25 years ago) and I’d pick the latter because it creates the map in one go while the former approach creates an empty one and copies and changes it three times afterwards. That being said, readability should override performance, at least in the first half of the project’s life. It’s your call.

As for case vs. separate function variants I prefer the latter because it gives me a separate semantic unit, namely something that deals with part of the whole logic. To me, separate smaller functions capture this idea quite well. And to me, that’s a better practice.

Another good practice is to avoid mutability and side effects / global state as much as humanly possible. Don’t reach for ETS until you are very positive – and backed this with actual data – that your app is having a hot spot of slowness (ETS happens to be an excellent cache). Don’t overuse files. Etc.

Since I am just about to go to bed, I can’t think of anything more right now. Do you have any other examples in mind?

2 Likes
7

This won’t work, as you guarded customer_id to be an integer, <>/2 does only work for binaries, you need to do something like "/customers/" <> Integer.to_string(customer_id) or "/customers/#{customer_id}".

2 Likes
8

The funny thing with Elixir - and why it has a place in my heart - is immutability everywhere and OTP - makes it hard to write “wrong” code.

Sometime it mightn’t look or smell right, but if it works, it’s kind of just a taste thing, not some subtletythat an overly naive coder has missed…

2 Likes
9

Even the pattern matching version can get smelly if you have too many keys at which point in time something like:

map_converter = fn (params, key_mappings) ->
  map_key_values = fn ({key,from}), key_values ->
    value = get_in(params,from)
    [{key,value}|key_values]
  end
  key_mappings
  |> List.foldl([], map_key_values)
  |> Map.new()
end
source = %{"key1" => "value1", "key2" => "value2", "something" => %{"nested_key" => "value3"}}
mappings = [
  key1: ["key1"],
  key2: ["key2"],
  key3: ["something","nested_key"]
]
result = map_converter.(source, mappings)

may become necessary.

Smells are context sensitive. And just because there is a smell right now that doesn’t mean it has to be addressed immediately - it should be addressed when it gets in the way (e.g. when the length of that pattern matching function head gets incomprehensibly long).

Traditionally “smells” were meant to suggest the applicability of certain refactoring recipies. But refactoring itself should be motivated by “making a necessary change easy”:

… not by the mere presence of a smell.

FYI: New edition: Refactoring: Improving the Design of Existing Code, 2nd Edition coming up. The second edition uses JavaScript instead of Java for the code examples.

3 Likes
10

I’d recommend using credo as well (if you aren’t already using it). It may point out some things that you hadn’t thought of. It was helpful for me when I started and its been useful in bringing in new developers to make them more self-aware of the type of code they are writing. https://github.com/rrrene/credo

6 Likes
11

For note, the whole {:ok, value}/{:error, reason} tuples are just monads, a good monad pipeline (like |>, but like the exceptional library uses of ~>) makes it significantly easier to read and use (and if elixir had it built in then such success tuples would no doubt be a lot more used in the ecosystem since they’d be so simple).

4 Likes
12

I think Elixir’s with macro help deal with pipelines of {:ok, value} | {:error, reason} tuples in a monadic way, i.e. fail-fast. In that sense, it is built-in.

4 Likes