Code Review: Is there a better/more clear way to write this ecto update function?

Questions / Help
,
1 
  def update_post_user_settings(post_id, community_member_id, params) do
    post_user_settings =
      case get_post_user_settings(post_id, community_member_id) do
        nil -> %PostUserSettings{community_member_id: community_member_id, post_id: post_id}
        post_user_settings -> post_user_settings
      end

    changeset = PostUserSettings.changeset(post_user_settings, params)

    case Ecto.get_meta(post_user_settings, :state) do
      :built -> Repo.insert(changeset)
      :loaded -> Repo.update(changeset)
    end
  end

So I think that this code is fairly straightforward to read/write. The tricky part is the determination of if I need to do a Repo.insert or a Repo.update. It’s very possible I’m missing out on an Ecto API that would make this code easier. I’m currently using Ecto 2.2.11 (haven’t been able to successfully upgrade to v3 yet).

1 Like
2

1)

The tricky part is the determination of if I need to do a Repo.insert or a Repo.update . It’s very possible I’m missing out on an Ecto API that would make this code easier. I’m currently using Ecto 2.2.11

Perhaps you are looking for: insert_or_update(changeset, opts)? According to the 2.2.11 docs:

The distinction whether to insert or update will be made on the Ecto.Schema.Metadata field :state . The :state is automatically set by Ecto when loading or building a schema.

2)

If you want to get tricky, your case statement can be replaced by:

post_user_settings = get_post_user_settings(post_id, community_member_id) 
                     || %PostUserSettings{
                          community_member_id: community_member_id,  
                          post_id: post_id
                        }

If the left side of || is nil or false, then the right side is evaluated and returned. If the left side of || is anything else, then the right side is not evaluated.

4 Likes
3

Ah, totally missed Repo.insert_or_update/2, that’ll be perfect. I think I’ll leave the case statement since I find it more readable/explicit in this case compared to ||.

This is what I’m going with:

  def update_post_user_settings(post_id, community_member_id, params) do
    post_user_settings =
      case get_post_user_settings(post_id, community_member_id) do
        nil -> %PostUserSettings{community_member_id: community_member_id, post_id: post_id}
        post_user_settings -> post_user_settings
      end

    PostUserSettings.changeset(post_user_settings, params)
    |> Repo.insert_or_update()
  end

I could even pipe all the way through but I don’t want to change the value traveling through the pipe that much.

4

Yep.

I could even pipe all the way through…

I was just working on a pipeline:

  get_post_user_settings()
  |> &Kernel.||( %PostUserSettings{
                  community_member_id: community_member_id,  
                  post_id: post_id
               })
  |> PostUserSettings.changeset(params)
  |> Repo.insert_or_update

Somewhat ugly.

5

In my judgement the only pipe-oriented refactoring that could possibly be justified is extract function:

  defp grab_post_user_settings(post_id, community_member_id) do
    case get_post_user_settings(post_id, community_member_id) do
      nil -> %PostUserSettings{community_member_id: community_member_id, post_id: post_id}
      post_user_settings -> post_user_settings
    end
  end

  def update_post_user_settings(post_id, community_member_id, params) do
    post_id
    |> grab_post_user_settings(community_member_id)
    |> PostUserSettings.changeset(post_user_settings, params)
    |> Repo.insert_or_update()
  end

Whether there is any benefit to that I think is somewhat subjective.

2 Likes
6

Me too. I posted an additional example of that, but then I deleted it–no use beating a dead horse. Although, if it was my code, refactoring get_post_user_settings() to return a default struct does appeal to my sense of style.