Is the explosion of "Utility" modules a problem or totally fine?

One of my teams with a .NET background has recently started working with Elixir. In c#, you have the concept of an extension method that allows you to extend an object’s methods without changing its code. I believe familiarity with this concept is causing the team to introduce a bunch of utility modules. Here are some examples:

defmodule EctoUtil do
  import Ecto.Query

  @doc """
  Filters the id from the query if the id is not nil
  """
  def maybe_exclude(query, nil), do: query
  def maybe_exclude(query, %{id: nil}), do: query
  def maybe_exclude(query, %{id: id}), do: query |> where([x], x.id != ^id)
end
defmodule MapUtil do
  @doc """
  Gets an element from a map by the key. Converts the key to or from an atom and tries again if no element is found.
  """
  def get(map, key, default \\ nil)

  def get(map, key, default) when is_map(map) and is_atom(key) do
    Map.get(map, key, Map.get(map, to_string(key), default))
  end

  def get(map, key, default) when is_map(map) and is_binary(key) do
    Map.get(map, key, Map.get(map, String.to_atom(key), default))
  end
end
defmodule ChangesetUtil do
  import Ecto.Changeset

  def validate_unique(changeset, fields, constraint_name, message, query \\ nil) do
    opts = [message: message, nulls_distinct: false]
    opts = if query != nil, do: Keyword.put(opts, :query, query), else: opts

    changeset
    |> unsafe_validate_unique(fields, My.Repo, opts)
    |> unique_constraint(:name,
      name: constraint_name,
      message: message
    )
  end
end

I have two questions really.

  • What do we think about these specific utility functions?
  • What is the general Elixir community consensus on utility modules?
1 Like

I tend to put utiltity modules under MyApp.Support.[Lib].[…] to not muddy up root namespaces with helper stuff.

More generally I’d suggest asking the hard questions though:

  • Is this truely generic (as in not related to an actual domain of the project rather than the domain of a library in use)
  • Is this a good idea
    • E.g I would argue that in general a map where you don’t know if the keys are atoms or strings is the problem, not the lack of an API checking both.
4 Likes

The only one I can constructively comment on is the MapUtil one. I think if you need this function then you are doing something wrong. People usually introduce this because they want to manipulate params coming from the web before passing them to a domain function. Really you should be casting them to a changeset immediately then operating on the changeset to make any adjustments.

The maybe_exclude also feels like a huge smell and that you may just be using Ecto wrong, but I’d have to see an example.

The validate_unique is fine. In terms of naming, though, I would just go with MyAppWeb.Changeset or MyAppWeb.Schema to avoid the rather useless Util suffix, but that’s just personal taste.

2 Likes

Delete the first two modules immediately :sweat_smile:.

2 Likes

On the same topic, I highly recommend boundary to help with this!

1 Like

Yeah, in functional languages, it’s all about data transformation, we use functions to do that. Utility modules are generally a red flag. But only in the context of coming from a different language, it tells us that the team is inexperienced.

Most of the time this comes down to design, separate of concern, unfamiliarity with OTP, and being bad at recursion.

I’ve seen a lot of engineers coming from other languages that tend to over abstract. Which is extremely bad for the codebase long term health. The best approach in my opinion is the 3-1 rule and keep everything in one file, until you are forced to abstract.

All that said, there has been some great responses in the thread already, which I would highly recommend to follow!

2 Likes
  • The maybe_* functions are IMO quite okay. They fit well with piping stuff and just accumulating state and/or a command object.

  • I agree with the others that MapUtil is masking a confusion about not wanting to deal with mixed or semi-invalid maps. That would never pass a code review from me. Convert maps from string-keyed to atom-keyed once at the edge (i.e. in your controller function) and just operate on that. Or, and I’ve seen that done a lot as well, just deal with the string-keyed map everywhere. It’s not such a huge inconvenience that some are making it out to be.

  • The validate_unique is alright.

  • The module names are indeed not optimal as others have also pointed out. They should just be called MyApp.Whatever and be left at that.

Is this best-practice? Should we not operate on the params, but first cast to changeset and then work with the changeset?

Best practice is in the eye of the beholder.

I prefer using changeset myself, as it gives a common interface and people are familiar with it vs your own home-brewed solution that every new developer has to decipher. Keep in mind that Ecto has the benefit of public scrutiny, which your company’s code does not.

3 Likes

I’m sure you’ll find people who say it’s fine to manipulate params—and like with everything, there are times it’s appropriate—but yes. The entire point of the Ecto.Changeset.cast constructor is to type cast untrusted data into a known shape with correct types (as opposed to its sibling change which is used when you have trusted data). Not only is is much easier to work with correctly typed data, it keeps all this logic in a single place. If you do some preparatory work on params first then past to cast, you’re spreading this logic out at different application layers. Changesets also come with lots of useful functions for working with changes, like being able to tell if a field is being changed or not.

1 Like

I think the “best practice” would be params with string keys represent data from outside the app, and should be treated differently than data with atom keys which must have originated from somewhere inside your app. Abstracting over this difference is problematic, because you are not abstracting something the two things have in common you are hiding an important distinction.

2 Likes

Agreed, this is exactly how I think about it. I’d be hard pressed to think of a scenario where you would legitimately want “indifferent access” as Rails calls it (there’s even a clue in Ecto in that cast won’t allow maps with mixed key types). If you find yourself needing this, there will be a different, and very likely cleaner, solution.

1 Like

It’s not about it being a best practice – don’t defer to foreign authority and utilize your own judgement. And would that tell you it’s a good idea to leave mixed and non-validated data fly around your entire app? I would not like that. I want parsing / validation to happen once and (1) have early errors if something can’t be properly utilized and (2) use the now-valid data everywhere else inside the app.

Ecto.Changeset gives you that, and more.

1 Like

While I completely agree with this, I can also understand where all this confusion comes from.

Most of us know, after writing a lot of elixir, how important is to validate data at the edge of the system, you basically get rid of the defensive style by having a explicit validation of data. We also know that ecto schemas are currently the de-facto tool to do this. For folk that just started with elixir this will never be obvious, especially the part where you can use ecto schemas/changesets without a database.

In the last years there is more coverage on this topic, but I still think it’s up to us to provide in form of blogposts/tutorials/books more awareness about this approach and the best ways to implement them.