Need Help how to refactor this code with Elixir to be more readable and to avoid nested cases

Questions / Help
,
1 
     def update(  %{
          full_name: full_name,
          password: password,
          password_confirmation: password_confirmation,
          email: email,
          language: language,
          time_zone: time_zone
        },
        %{context: %{user: %{uid: user_id}}}
      ) do
    user = User |> Repo.get(user_id)

    verif_email = user.email === email

    case verif_email do
      
      true ->
           case AccountClient.account_update(
               user_id,
               full_name,
               password,
               email,
               language,
               time_zone,
               password_confirmation
             ) do
          {
            :ok,
            %{
              data: %{
                attributes: %{
                  full_name: full_name,
                  email: email
                },
                id: uid
              }
            }
          } ->
            {:ok, User.update!(uid, email, full_name)}

          err ->
            err
        end


      false ->
     
        with false <- User.check_email(email),
             {
               :ok,
               %{
                 data: %{
                   attributes: %{
                     full_name: full_name,
                     email: email
                   },
                   id: uid
                 }
               }
             } <-
               AccountClient.account_update(
                 user_id,
                 full_name,
                 password,
                 email,
                 language,
                 time_zone,
                 password_confirmation
               ) do
          {:ok, User.update!(uid, email, full_name)}
        else

          true ->
            {:error, %{message: "Email has been already taken .."}}

         
          err ->
            err
        end
    end
  end
2

Hello and welcome,

Please add code fence ``` around your code, like for markdown text. It will be more readable :slight_smile:

3

thank you ! Done :slight_smile:

4

Some considerations…

  • Don’t use case with true/false… use if
  • Try avoiding local var if not needed
verif_email = user.email === email
case verif_email do

#instead, try this
case user.email === email do...

# or much better
if user.email === email, do: ..., else:...
  • Use with with nested conditions

I see You have used one… did You write this code?

2 Likes
5

You want to separate out the various kinds of conditions you’re checking here.

  • Checking for the users email: is is the users email, is it another and unused, is it another, but already used?
  • Was the account update successful
  • Updating the user based on the account update.
def update(map, %{context: %{user: %{uid: user_id}}} ) do
  user = User |> Repo.get(user_id)
  
  cond do 
    user.email === email -> update_account(map, user_id)
    !User.check_email(email) -> update_account(map, user_id)
    true -> {:error, %{message: "Email has been already taken .."}}
  end
end

defp update_account(map, user_id) do
  case account_update(map, user_id) do
    {:ok, account_result} -> {:ok, user_update(account_result)}
    err -> err
  end
end

defp account_update(map, user_id) do
  AccountClient.account_update(
    user_id,
    map.full_name,
    map.password,
    map.email,
    map.language,
    map.time_zone,
    map.password_confirmation
  )
end

defp user_update(account_result) do
  %{
    data: %{
      attributes: %{
        full_name: full_name,
        email: email
      },
      id: uid
    }
  } = account_result
  
  User.update!(uid, email, full_name)
end
9 Likes
6

yes of course i did for sure but it was not like that ! it was with if conditions but i changed it … i appreciate your note :slight_smile:

7

Thank you so much for your help ! this so readable and so clear ! :smiley: :smiley: