How would you refactor/simplify this?

Phoenix Forum Questions / Help
, ,
1

Hello, beginner here and looking for possible code refactor/simplification:

# UserController
def index(conn, _params) do
  users = Accounts.list_users() |> Accounts.user_preload_role()

  # can I pipe this into the above instead?
  # basically want superusers hidden from non super-users
  users =
    if !conn.assigns.is_super_user do
      users |> Enum.filter(fn u -> !u.role.is_super end)
    else
      users
    end

  render(conn, "index.html", users: users)
end

# Accounts context
def list_users do
  Repo.all(User)
end

def user_preload_role(user), do: Repo.preload(user, :role) # is preloading this way dumb?

I’m interested in how you would approach this problem.

Thank you.

2

Probably something like

def index(conn, _params) do
  users 
    = Accounts.list_users()
    |> Accounts.user_preload_role() 
    |> Enum.filter(fn user -> 
          should_user_be_shown(user, conn.assigns.is_super_user)
        end)

  render(conn, "index.html", users: users)
end

defp should_user_be_shown(_user, true=_is_admin), do: true

defp should_user_be_shown(user,_is_admin), do: !user.role.is_super_user

Makes the logic test for whether a use should be shown more explicit, plus you can pipe it. You could move the preload call into Accounts.list_users_with_roles. You could also use the & notation for calling functions in the Enum.filter call to reduce a bit of visual clutter, but typing this out on iPhone has been a bit tedious so I’ll leave that to you!

1 Like
3

In addition to what has been said, I would prefer to filter at the db level, and not on the result of a db call… Imagine You also want some pagination, this will break.

And this is working, but it does not really work as You think…

def user_preload_role(user), do: Repo.preload(user, :role)

In fact You pass a list of users, not a single user.

To make it work at db level, You need to know Ecto, and pass some criteria.

This can be done by separating list_user in list_user_query. Something like this…

users =
    if conn.assigns.is_super_user do
      Accounts.list_users()
    else
      Accounts.list_users(admin: false)
    end
    |> Accounts.preload_role()

def preload_role(user_or_list), do: Repo.preload(user_or_list, :role)

def list_users(criteria \\ []) do
  criteria
  |> list_users_query()
  |> Repo.all()
end

def list_users_query(criteria) do
  ... # hint, use Enum reduce on criteria
end
4

Just do everything in Ecto, without additional filtering in the controller:

only_superuser = conn.assigns.is_super_user

Repo.all(
  from u in User,
    inner_join: r in assoc(u, :role),
    where: ^only_super or r.is_super
)
1 Like
5

I think you should avoid to add business logic in your controller.
Your action should look something like this:

def index(conn, _params) do
  users = Accounts.list_users_visible_for(conn.assigns.current_user)
  render(conn, "index.html", users: users)
end

And you should add that method to the context, so it looks something like this:

  def list_users_visible_for(user)
    if super_user?(user)
      list_users |> user_preload_role()
    else
      list_non_super_users()
    end
  end
3 Likes
6

I like to have a separate query, because it allows filtering, but also pagination, preload…

  def list_users_query(criteria \\ []) do
    query = from p in User

    Enum.reduce(criteria, query, fn
      {:limit, limit}, query ->
        from p in query, limit: ^limit

      {:offset, offset}, query ->
        from p in query, offset: ^offset

      {:admin, false}, query ->
        # your code to filter non admin user

      {:order, order}, query ->
        from p in query, order_by: [{^order, ^@user_order_field}]

      {:preload, preloads}, query ->
        from p in query, preload: ^preloads

      arg, query ->
        Logger.info("args is not matched in query #{inspect arg}")
        query
    end)
  end

This allows to use this kind of query

Accounts.list_users(admin: false, preload: :role, limit: 10, offset: 0, order: :asc)
5 Likes
7

Thanks for the answers. Takeaway is: have thinner controllers, move business logic as much as possible to context (since it can be reused for other controllers as well), do more with Ecto. I’m glad I asked.

3 Likes
8

I built on top of answers from @nicanor and @kokolegorille and came up with:

# UserController

def index(conn, _params) do
  users = Accounts.list_users_visible_for(conn.assigns.current_user, preload: [:role])

  render(conn, "index.html", users: users)
end

# Accounts context

def list_users(criteria \\ []) do
  criteria
  |> user_query()
  |> Repo.all()
end

def list_users_visible_for(%User{} = user, criteria) do
  criteria
  |> Keyword.put(:is_super, user.role.is_super)
  |> list_users()
end

# renamed to user_query (from list_users_query) because I use this for get_user too
def user_query(criteria \\ []) do
  query = from(u in User)

  Enum.reduce(criteria, query, fn
    {:is_super, false}, query ->
      from u in query,
        inner_join: r in assoc(u, :role),
        where: not r.is_super

    {:is_super, true}, query ->
      query

    {:preload, preloads}, query ->
      from u in query, preload: ^preloads

    arg, query ->
      Logger.info("args is not matched in query #{inspect(arg)}")
      query
  end)
end

Thanks again for your inputs.

2 Likes