Good and Bad Elixir
keathley.io
keathley.io
How often is it that you don't know if you're expecting a map or a keyword list? I don't think I've ever intentionally put myself in that situation (though it's nice that access supports both).
They say to avoid piping result tuples, but I actually think their example code shows the strength and weaknesses of both approaches. I use pattern matching on tuples when you want more complicated behavior (that might be hard to read in a big else block for a with) and a nice with statement when there's 'one true path.'
In general, I think one of my favorite things about Elixir is that they've done a good job of avoiding adding features you should never use. These all feel like personal judgements about tradeoffs rather than actual hard rules.
The sort of declarative good/bad language worries me a bit because people new to the language may take it as gospel, but overall I’m glad to see people putting their thoughts out there with explanations and examples :)
If I ran across a lot of these suggestions in code, I'd refactor them to be more idiomatic/practical.
Right now I'm basically wrapping basically all my data in Agents with maps inside, but that is so lame.
Thanks for the quick response. Maybe I'm trying too hard to use a saw to hammer a nail.
EDIT: I don't care about type signatures on functions. I'd just like to be able to have a real time system that updates based on ingested values, that I can distribute easily. It seems like it's possible, even close, but the structs just don't get me quite where I want them to be.
That is, by defining an Ecto schema for your data structure you can go almost directly from raw json to clean, correctly casted data.
To use an example similar to yours, this is how I process data from cryptocurrency exchange APIs—just write up a schema for the shape of the response, parse the json into a map and throw the map into Ecto.Changeset.cast. Even if everything is a string in the raw data, it will be correctly cast into integer/float/whatever as defined in the schema.
On another note, I find Dialyzer a bit slow to run even on modest codebases.
> You should not use else to handle potential errors (when using with)
Yes you should! Otherwise the with statement could return something that is not :ok and also not :error. I think therefore it’s good practice to put else {:error, error} -> {:error, error} especially because Elixir is not strongly typed so this helps constrain the possible types the statement can return. If you omit the else, your construct can return any number of types from values produced downstream.
It's just programming to the interface and not the implementation, which often has advantages and very rarely has any downsides.
Access works with maps and keyword lists, but not structs. There are pieces of code I've written that reasonably accept map or struct, but not keyword list.
Map.get/3 also has a default value; Access only lets you use `nil` for that.
OTOH, the nil-eating behavior of Access is terribly convenient sometimes.
It's all about what you need at the time. There is more than one way to do it, for a reason.
I'm glad it came across this way. These are all ideas that I believe have helped me build more maintainable systems. But that's just my experience and opinion. I'm happy people are finding stuff to disagree about because I also want to grow and improve my own ideas.
I regret titling the post with the words "good" and "bad". This started as an internal memo for my new team, grew into blog post and I didn't change the title. The title adds connotations that aren't very useful to the reader and sets up a more confrontational tone then I want. Live and learn.
Either way, thanks for taking the time to read through it and for providing feedback.
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 :).
I bit off topic, but this sums up a lot of my experience with code reviews. People have opinions on what is the "one right way" and if I differ, we go through this drawn out discussion on Github about it. Most of the time, I see it as there are different ways to do it with different trade offs. I find myself just giving in so I can get the damned PR approved without a few days of back and forth.
Granted there are times when there is a much better way. In those cases, I very much appreciate the feedback. It's truly educational. In my experience though, most of the time, it's senior developers insisting their style/way is best.
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.
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.
Where this gets hard for me, is in transitioning between teams, contributing to open source projects etc. The dialects can get kind of discordant. At least with Elixir, I find these differences are more palatable because the basic paradigm is consistent. In the "hybrid" languages, you get competing language models in the same code base.
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.”
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...
A much more basic example of this from my recent experience is discovering GenStage, which solves the generic problem of backpressure in sequential processing but doesn’t appear in the core Elixir documentation.
The reason is that the underlying data structures are quite different, and operations and access patterns on them shouldn't be agnostic.
You should use Keywords basically only as options to functions, in my opinion, and that's just for historical consistency and interop with Erlang. Otherwise key-value pairs should be a map. And using `Keyword.get` and `Map.get` make it immediately obvious what the underlying data structure is, as well as prompt you to think about whether you should use the `!` version or provide defaults.
Otherwise, I think the tips are all decent to great. I am a little on the fence with exceptions, though. I know there's a meme in the community of "let it crash", but in my experience, that rarely feels like the right thing to do. I much prefer a Rust-style, "Result-all-the-things". It seems hard, in practice, to rely on crashes and restarts to maintain the right state invariants.
I feel the opposite way. Perhaps if you are mostly doing things where you have control over a transient state (like a chain of validations on a database entry) the error tuples are easy...
But if you are doing something like checking in on cached state for something remote that you don't have control over... It is so, so much better to give up, restart your GenServer, and re-sync your cached state in the init() clause than try to "guess" what the remote thing has for its state after it's sent back an error.
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.
The same would be true if you were building a kafka consumer. You wouldn't want to crash in that scenario since you could easily poison the entire topic with one bad message.
There are ways to allow for crashes this way though. An alternative approach would be to use a dedicated process for scheduling work and a dynamic supervisor that can be used to start workers as needed. The work scheduler would monitor these worker processes. This means that you could freely crash the worker and allow the work scheduler to determine what further action must be taken. I've used both your approach, and this alternative approach in the past both to good effect.
I learned this the hard way. I wrote a GenServer that does some periodic database cleanup (maybe not the best solution), and I naively wrote the init function to immediately do the first cleanup on startup. Then I discovered a case off the happy path where the cleanup was failing due to a constraint violation, and the failing init brought down the whole application (edit: while someone was trying to use it, of course).
handle_continue was added for these kinds of things that should happen when starting up gen_ processes.
the entire init chain is sequential, so not only will it risk crashing your entire supervisor tree, but it will slow startup
But, allowing the process to crash doesn't punt retry logic at all. For instance, in the work scheduler example, the work scheduler would detect that a worker had crashed and would back off appropriately. This allows you to write your worker code in a way that avoids error handling while still managing faults.
In particular: "14:55 - Engineers pinpoint the issue to be strongly correlated to a spike in errors in originating from our service discovery modules. It is determined that the service discovery processes of our API service had gotten into a crash loop due to an unexpected deserialization error. This triggered an event called "max restart intensity" where, the process's supervisor determined it was crashing too frequently, and decided to trigger a full restart of the node. This event occurred instantaneously across approximately 50% of the nodes that were watching for API nodes, across multiple clusters. We believed it to be related to us hitting a cap in the number of watchers in etcd (the key-value store we use for service discovery.) We attempt to increase this using runtime configuration. Engineers continue to remediate any failed nodes, and restore service to our users."
What's keeping you from implementing a back off strategy in your init? You can return :ignore to the supervisor and try again later.
If you're backing off inside the process itself, you're gonna get some weird shit where your service exists but it's not in a truly available state, and you will have to handle that by basically halfheartedly rewriting logic that already exists in OTP.
- I like &Map.get/3 over the some_map[:some_key] syntax
- &with/1 can be hellza confusing in many case if you're calling a bunch of functions, and any ol' one of them can return an error message (e.g. where the heck did `{:error, :bad_arg}` come from?!
- piping into case statements is awesome, and I'll only stop doing it when people complain about it
- higher order functions are useful, and I don't see a problem using one's judgement as need on when to use them
- In the cases where using &with/1 is actually useful, so is using the else block if you really need to
...
Yeah, I just don't like this article's recommendations.
The equivalent of do-notation would be with statements I think: https://elixirschool.com/en/lessons/basics/control-structure...
You agree with the article on this point:
with is best used when you can fall through at any point without worrying about the specific error or contrary pattern.
Higher-order functions are great, so try not to hide them away. If you’re working with collections, you should prefer to write functions that operate on a single entity rather than the collection itself."
Excellent advice -- and not just for Elixir. I find myself applying this technique often in Python, for example. While the example is fairly simple and contrived, the consequences they note ring true.
[0]: http://www.gar1t.com/blog/solving-embarrassingly-obvious-pro...
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.
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).
For me, it doesn't mean "thou shalt not use if-then ever" just that if you have enough conditionals and logic needed, pattern matching is better suited for those cases (sorry for the pun).
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.
You'd do the exact same thing if you were testing a series of pattern-matched functions. The tests would be unchanged either way.
The biggest challenges for elixir currently are the lack of established HTTP libs with dynamic testing and db ORMs. For instance, interacting with PSQL is fine using ecto, but using a document based DB or something like Neo4J is an absolute headache. Deployment is also pretty easy, but that's a whole different side of development.
It wasn't long ago where the only posts in here would describe it as the biggest headache!
Mmmm. No. It makes sense into context of the example, but this is way too generic advice it's actually bad.
Pipes are awesome. Please _do_ pipe results into the following function when it makes sense to.
I think they specifically said, when piping, don’t force functions to handle the error of the upstream function.
Because each error needs to be handled differently, the downstream function’s implementation becomes tied to the upstream function. Then the downstream function is only usable with said upstream function.
I sort of disagree with Access vs Matching vs Map; if I change my data structure I'm changing the contract I've established. I want things to break in that case, and I want them to break as fast as possible. This seems like great advice for a controller action, not so much for a gen_server or anywhere I'm NOT dealing with user input.
And... I completely disagree with "Raise exceptions if you receive invalid data." If you receive invalid data return an error saying that... please for the love of someone's god, do not raise an exception. You have no idea what the concerns of the caller are, so just return an error tuple.
> This allows us to crash the process (which is good) and removes the useless error handling logic from the function.
No, the exception returned will likely have no meaning to the client and will more than likely confuse them or your. You will still have to write error handling logic. You still have to format the error. You still have to log or report the error... Now you are doing it in multiple places because you're raising and likely still having to handle data validation issues. The best of both worlds, IMHO, is returning an `{:error, exception}` tuple.
Edit: Forgot the "NOT" in the end of the second paragraph.
Let it crash works well for a phone system where it doesn’t make sense to call both parties back if the thing failed.
> No, the exception returned will likely have no meaning to the client and will more than likely confuse them or your. You will still have to write error handling logic. You still have to format the error. You still have to log or report the error... Now you are doing it in multiple places because you're raising and likely still having to handle data validation issues.
If it's invalid in the sense of indicating a bug in the caller (e.g. by violating your function's preconditions), raising an exception is the right thing to do. You don't understand the problem, and probably the only way it can be fixed is by the developer fixing it. Precisely because you have no idea what the concerns of the caller are, each caller needs to do its own validation and error handling logic.
If it's a case of "invalid" input but valid code (e.g. a function that's designed for parsing user input), then that's the case where doing validation yourself and returning an error is better.
(This kind of distinction is where it's really helpful to have a type system, so you can use separate types for raw input and parsed data that's expected to be valid).
(I bit off more than I can chew by trying to write a full-featured API for an embedded GPS device that speaks a proprietary binary protocol - it's been a slow project)
Easier to talk about where I disagree, but overall great read!
I also think it looks pleasing, plus it’s easy to read and it avoids allocating a new variable compared to the alternative example given.
I'm certainly not against using those functions and the additional checking can be of use in certain circumstances. The use case I had in mind was specifically how people will use `Keyword.get` to grab configuration values or options. This means that if you load configuration from a json file or external service, and it comes back as a map, you now need to convert to a keyword list just to initialize a library. Its a small thing, but in those situations I would rather allow for more flexibility in the interface.
That said, I think this advice is particularly useful for people building libraries more so then people working on applications. I may try to add more nuance to this point in the post.
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.
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.
1. The argument the author makes here assumes that else clauses can or should only used be for error handling. There are instances where you might want to drop out of a with statement without returning an error however. Some examples might include connection Plugs that don’t need to perform some behavior due to certain assigns already being present on the Connection struct or configurable / optional behavior encoded in an options list. In these instances, pattern matching on a successful fallback clause actually reduces complexity as it avoids the need to position that branching logic somewhere outside of the main with statement that it is actually concerned with.
2. I find error handling through with / else and annotated tuples to actually lead to very expressive, easy to read code. Having built and maintained a Phoenix application in production since 2015, this is actually a very common pattern I and other members of my team use so I’m curious as to why the author considers it something you do under “no circumstances”.
General consensus is that there's a lot of things pointed out here that are good to think about and consider, but don't treat them all as gospel.
Some situations are better handled by other approaches, but understanding the goals of why the author has pointed these out will be beneficial for everyone.
Definitely not gospel. These are patterns that I tend to use and believe have helped increase the maintenance of the various projects I've worked on. But I'm also guilty of going against these points when it seems appropriate.
1 - avoid if else {} blocks, use `cond do .. end` blocks instead, they're more readable
2 - avoid importing modules, but rather use them directly wherever possible, this will help when refactoring code or to find all occurrences of a particular Module.method()
3 - when specifying keyword list options, explicitly supply [ ] in the arguments, example: Abc.method(a: 2, b: 3) may be clearly written as Abc.method([a: 2, b: 3]) so we always know it uses a keyword list and not map
4 - Use a file `.iex.exs` to specify all your aliases and macros you may need in your Elixir shell. Helps a lot while you're playgrounding in the IEx shell. In my case, I have aliased all my Context modules and Model modules
5 - Pattern match everywhere. This is one of the powers of this language. Make your methods follow the {:ok, data} or {:error, message} pattern and it's already very maintainable.
Have you ever tried to debug something in iex, but database calls and other log messages keep stomping all over your prompt? You can stop them temporarily calling the function
Logger.remove_backend(:console)
Logger.add_backend(:console)
defmodule Clog do
def stop do
Logger.remove_backend(:console)
end
def start do
Logger.add_backend(:console)
end
end
Clog.stop
Clog.start
You can call your module whatever you'd like. I just called mine "Clog" because the log messages were always clogging up my console. I hope this helps.
if function_exported?(Mix, :__info__, 1) and Mix.env() == :dev do
Logger.configure_backend(:console, device: Process.group_leader())
end
Does Elixir have anything akin to Haskell's "do notation" for result types? The provided best practice code feels verbose, and that surprises me given that Elixir looked clean & modern.
For example
# Do...
def main do
with {:ok, response} <- call_service(data),
{:ok, decoded} <- parse_response(response) do
decoded
end
end
Result<T> Main(object data) {
return from response in call_service(data)
from decoded in parse_response(response)
select decoded;
}call_service could just return response and remove the verboseness.
so it would be:
response <- call_service(data)
I don't know C# but would that code explode if there was an error, instead of in again the soft error pattern likely getting {:error, foo} ?
Regardless my point is that {:ok, bar} stuff isn't a language required aspect it's a community pattern if that makes it any better for you.
It is not a community pattern, because it's the way OPT (Erlang's "standard library") functions work e.g. maps:find? returns `{ok, Value} | error`.
And Elixir inherited that rather directly and build its standard library the same way. `Map.fetch` similarly returns `{:ok, value()} | :error`.
It's not hard-coded into either language, but it's absolutely not "community adopted" or "a community pattern" either.
What would you call something then that clearly has optional adoption and isn't consistently used across the standard library?
Unless you use Map.pop! which raises an error, and because pop! raises an error I would then expect Map.pop to return {:error, nil} or {:error, Map} or something instead of {nil/Default, Map}.
I won't try to back up where/if there's more this is but it was the one I had in mind while writing.
By having a version that raises the standard library is basically saying not having a key is the error state.
Yeah, sorry. I should have been more clear. This behavior is common in Lisps, which is what I’m used to. It’s less common in mainstream languages. With Elixir it may be Clojure-inspired.
> By having a version that raises the standard library is basically saying not having a key is the error state.
Not necessarily, it’s saying you can choose whether it should be an error or not. Notably other functions that error don’t give you the option of specifying a default. That’s the difference here. Nil is returned because it’s passed in, it’s the default argument for the “default value” parameter.
def main do
with {:ok, response} <- call_service(data),
{:ok, decoded} <- parse_response(response) do
decoded
end
end
That being said, my main issue with nested 'with' (or nested 'case', for that matter), is that nothing helps you make sure you handled all the possible way for any of the nested function to fail.
And someone, someday, is going to add another way for one of the functions to fail, and you will not have written a handler for that because you're not clairvoyant. And you won't notice until it crashes in prod, and your customer complains.
Good advice overall, otherwise !
Practical, real world, and takes enough of a stand to wake up the pedants.
require OK
OK.for do
user <- fetch_user(1) # `<-` operator means func returns {:ok, user}
cart <- fetch_cart(1) # `<-` again, {:ok, cart}
order = checkout(cart, user) # `=` allows pattern matching on non-tagged funcs
saved_order <- save_order(order)
after
saved_order # Value will be wrapped if not already a result tuple
endI’d wondered about the pipes construct in Elixir for exactly that reason. Sure, it looks nice and easy, but based on my experience with Erlang it seemed unlikely to be useful unless you are simply planning on crashing for any errors (which isn’t really the Erlang way).
Map.get(foo, :bar, :some_default_value)
foo[:bar]
I also disagree with not using else in a with statement. That advise seems a lot like, "Next, pull out your pistol and shoot yourself in the foot," to me.
Languages have idioms for a reason and a lot of these suggestions go against idiomatic Elixir, which I'd recommend against.
iex(1)> foo = %{a: 1, b: "test"}
%{a: 1, b: "test"}
iex(2)> foo[:bar] || :some_default_value
:some_default_value
iex(3)> foo = [a: 1, b: "test"]
[a: 1, b: "test"]
iex(4)> foo[:bar] || :some_default_value
:some_default_value
Also Map.get/3 and Keyword.get/3 are probably faster.
I think there is a limit to how many things you should handle in a with statement, but the same goes for regular pipes as well.
Instead of this...
with {:ok, response} <- call_service(data),
{:ok, decoded} <- parse_response(response) do
decoded
end
{:ok, response} = call_service(data)
{:ok, decoded} = parse_response(response)
decoded
data
|> call_service!() # don't bother returning ok tuples
|> parse_response!() # just use bang methods!You shouldn’t have to unpack complexity just to understand where a test failed or whether it failed because of bad test code or bad tested code.
Once you have so much complexity that reading and mentally executing the test takes time, you should probably be writing tests of your unit tests.
BUT, I did like seeing someone’s point of view with thoughtful explanations and examples, kind of like what you get out of a good discussion at an elixir meetup or something :D I enjoyed reading it.
can you elaborate more on the "Don’t pipe into case statements" point though? Not entirely what you're saying with this one as there isn't any explanation why one might be better/worse than the other, other than aesthetic/personal preference. Needing to access the result from the previous pipe in the case maybe?
iex(2)> :something |> if do true else false end
true
iex(3)> nil |> if do true else false end
false