Refactor tips for simple function for more idiomatic Elixir

Questions / Help
1

I’d love for Ecto.Repo.get or get_by to return a {:ok, foobar} tuples. I could use it in with conditionals for cleaner more idiomatic Elixir.

Here’s my function:

def can_list_bots?(team_id, user_id) do
  team = Repo.get(Team, team_id)
  user = Repo.get_by(User, id: user_id, team_id: team_id)

  if team != nil && user != nil do
    "admin" in user.roles
  else
    false
  end
end

Is there any way to make this code more idiomatic that I’m not seeing?

Thanks for the tips!

2

Maybe

def can_list_bots?(team_id, user_id) do
  team = Repo.get(Team, team_id)
  user = team && Repo.get_by(User, id: user_id, team_id: team_id)

  if user do
    "admin" in user.roles
  else
    false
  end
end

or if you want with

def can_list_bots?(team_id, user_id) do
  with team when not is_nil(team) <- Repo.get(Team, team_id),
       user when not is_nil(user) <- Repo.get_by(User, id: user_id, team_id: team_id) do
    "admin" in user.roles
  else
    _ -> false
  end
end

or with with pattern matches

def can_list_bots?(team_id, user_id) do
  with %Team{} <- Repo.get(Team, team_id),
       %User{roles: roles} <- Repo.get_by(User, id: user_id, team_id: team_id) do
    "admin" in roles
  else
    _ -> false
  end
end

You can also do it in a single sql query. Probably.

6 Likes
3

with/1 can use guards

team when team != nil <- Repo.get(Team, team_id)
4 Likes
4

Maybe I would start by removing if…

def can_list_bots?(team_id, user_id) do
  team = Repo.get(Team, team_id)
  user = Repo.get_by(User, id: user_id, team_id: team_id)
  do_can_list_bots?(team, user)
end

defp do_can_list_bots?(nil, _), do: false
defp do_can_list_bots?(_, nil), do: false
defp do_can_list_bots?(_, user), do: "admin" in user.roles

But I don’t pretend to be a syntax expert, and the code is not tested.

I agree using with is the Elixir way.

5

This one gets my vote - especially on the User structure to snatch (destructure) the roles unless

do it in a single sql query.

:slight_smile:

6

Thanks for the tips guys! I really like the with guard clause with when not is_nil.

7

Slightly off topic but do you really need to fetch the team at all? It seems like if you can assume that a team exists if the user has that team_id then you don’t need to fetch the team at all.

8

Haven’t tried it but maybe this single query could work:

Repo.one(from u in User,
  where u.team_id == ^team_id,
  where u.id == ^user_id,
  where "admin" == fragment("ANY (?)", u.roles)
)
2 Likes
9

+1

Yeah the more you can do in a single DB call, generally the better.

10

Remember that your app’s Repo is still a module. Feel free to define a fetch function that converts get calls into success tuples.

In the primary app I work on there are fetch* functions that cover get, one, etc. All to make pattern matching in with chains clearer.