Clean code: how to refactor these two basic ecto queries?

My code is working however; I am trying to level up as an engineer. How would refactor these queries so I practice DRY and do not repeat myself? THank you!

def example_function_1(user_id) do
  state = :red

  query =
    from(u in User,
      join: a in assoc(u, :accounts),
      join: p in assoc(a, :places),
      where: u.id == ^user_id and p.state == ^state,
      limit: 1,
      select: count(p.id)
    )

  Repo.one(query)
end

def example_function_2(user_id) do
  state = :red

  query =
    from(u in User,
      join: a in assoc(u, :accounts),
      join: p in assoc(a, :places),
      where: u.id == ^user_id and p.state == ^state,
      order_by: [asc: p.inserted_at],
      limit: 1,
      select: p.inserted_at
    )

  Repo.one(query)
end

Ecto queries are composable. You can define a query with only the shared clauses, and extend it for other purposes. For your case, you can try:

shared = 
    from(u in User,
      join: a in assoc(u, :accounts),
      join: p in assoc(a, :places),
      where: u.id == ^user_id and p.state == ^state
    )

# extended query for example function 1
from([u, a, p] in shared, select: count(p.id), limit: 1)

# extended query for example function 2
from([u, a, p] in shared, select: p.inserted_at, order_by: [asc: p.inserted_at], limit: 1)
9 Likes

instead of from…count just do

shared |> Repo.aggregate(:count)
1 Like

That returns the number of users not places

It is okay to repeat yourself, when it makes sense!

Its okay to repeat fragments of the queries if the queries are semantically unrelated and just happen to share some pieces by accident.

Do not refactor just to fullfil DRYness! It will get you somewhere, where you do not want to be.

Sandy Metz once said “duplication is cheaper than the wrong abstraction” and she is so right with it!

8 Likes

I prefer code duplication as well but get burned for it at work. How do you argue for this at work?

It seems like DRY is a religion.

I usually simply refuse to abstract with the comment “having no abstraction is better than having the wrong abstraction”.

Then they are free to either merge as is, merge with doing the changes their self or drop the MR that I invested time into.

What ever they do, either time gets lost or they appear in the git blame, both are things that they usually do not want.

Though I’m in a position where I know that I can be “rebellious”. And the team knows, that when I see the opportunity for an abstraction that makes sense, then I will usually do it in a preparational MR for the actual work to do.

3 Likes

As a junior engineer who cannnot afford to lose their job, refusing to make the changes is not an option lol. Thank you though Nobbz! It makes me feel less crazy that I dislike DRY just because it’s repeated… not useful abstractions sometimes IMO.

You shouldn’t be threatened with losing your job when refusing to religiously follow advice that’s NOT universal.

Sounds like a toxic workplace. :confused:

2 Likes

One useful technique where I try to avoid wrong abstraction is to see whether is the end user for 2 different methods the same group of users.

For example, you have a marketplace app where there are buyers and sellers. Somehow, there are certain code duplications for similar features that apply to both buyers and sellers.

In this case, I will defer the refactoring and abstractions as chances are, the code just happen to be the same, and there’s a high chance that this might diverge in the future.

Another principle I use is instead of DRY, I use WET (write everything twice). I am fine with one duplication of the exact same code. But when it happen another time, I will assess whether is there a need to refactor.

4 Likes

I’ve found it sometimes helpful to point people at this.

4 Likes

Duplication is generally a bad thing in software. We don’t like duplicated code. When code is truly duplicated, we are honor-bound as professionals to reduce and eliminate it.

But there are different kinds of duplication. There is true duplication, in which every change to one instance necessitates the same change to every duplicate of that instance. Then there is false or accidental duplication. If two apparently duplicated sections of code evolve along different paths—if they change at different rates, and for different reasons—then they are not true duplicates. Return to them in a few years, and you’ll find that they are very different from each other.

[…]

When you are vertically separating use cases from one another, you will run into this issue, and your temptation will be to couple the use cases because they have similar screen structures, or similar algorithms, or similar database queries and/or schemas. Be careful. Resist the temptation to commit the sin of knee-jerk elimination of duplication. Make sure the duplication is real.

–Martin 2018. Clean Architecture. p154.

1 Like