Piping Map.put with access to the piped value

Questions / Help
1

Hey elixirists,

another of my questions regarding piping:

The following code works perfectly well for what I want to achieve, but I don’t like the reuse of the var - it looks like I want to use pipes. I have not been able to make it work as I need to access the piped value in between. How can I improve the code?

def show(conn, %{"id" => id}) do
    project = Clients.get_project!(id)
    project = Map.put_new(project, :budget, total_budget(project.budget_items))
    project = Map.put_new(project, :budget_used, budget_used(project.work_items))
    project = Map.put_new(project, :budget_remaining, project.budget - project.budget_used)

    changeset = Clients.change_budget_item(%BudgetItem{project_id: project.id})
    render(conn, "show.html", project: project, changeset: changeset)
  end

Thanks :slight_smile:

2

Look up Map.merge/3. Probably something like:

project = Map.merge(project, %{
  budger: total_budget(project.budger_items),
  budget_used: budget_used(project.work_items),
  budget_remaining: project.budget - project.budget_used
}, fn _, existing, _ -> existing end)

Also maybe extract it out as a separate function (get_or_add_budgets ?) in Clients context or something?

3 Likes
3

Instead of reusing the variable you can take Map.put and wrap it into three separate functions. Each function
will update the struct. Not sure what your struct looks like so I’ll just use %Project{}

def put_budget(%Project{budget_items: budget_items} = project) do
  Map.put(project, :budget, total_budget(budget_items))
end

def put_budget_used(%Project{work_items: work_items} = project) do
  Map.put(project, :budget_used, budget_used(work_items))
end

def put_budget_remaining(%Project{budget: budget, budget_used: budget_used} = project) do
  budget_remaining = budget - budget_used
  Map.put(project, :budget_remaining, budget_remaining)
end

Now you can set up a pipeline for your struct.

def show(conn, %{"id" => id}) do
  project = Clients.get_project!(id)
  |> put_budget()
  |> put_budget_used()
  |> put_budget_remaining()

  changeset = Clients.change_budget_item(%BudgetItem{project_id: project.id})
  render(conn, "show.html", project: project, changeset: changeset)
end
4 Likes
4

I agree that Map.merge would be good to use here, I just would like to point out that it will override any existing values (unlike Map.put_new). But I would probably restructure the code so that Map.merge would work well.

5

Thanks everyone, my current approach is now:

  def show(conn, %{"id" => id}) do
    project = Clients.get_project!(id)

    total   = total_budget(project.budget_items)
    used    = budget_used(project.work_items)

    project =
      project
      |> Map.put_new(:budget, total)
      |> Map.put_new(:budget_used, used)
      |> Map.put_new(:budget_remaining, total - used)

    changeset = Clients.change_budget_item(%BudgetItem{})
    render(conn, "show.html", project: project, changeset: changeset)
  end

I still quite like @ericgray s idea of splitting this up into separate functions. I think I would go this way.

6

I think here the function getting passed to Map.merge/3 makes sure there are no overrides.

Just judging from the code it seems odd to me that you wouldn’t want to override existing values (especially if project is a struct), but the OP says that works for them so I’ll take them at their word.

I also want to +1 the recommendation to put all of this in a function in a context module as opposed to in a controller, which is what appears to be happening.

1 Like
7

Ah, good point, you’re correct. I didn’t notice that it was Map.merge/3 being used instead of Map.merge/2.

1 Like
8

For what it’s worth, I might do:

# In the Clients module
defp add_budget_data(project) do
  budget  = total_budget(project.budget_items)
  budget_used = budget_used(project.work_items)
  budget_remaining = total - used

  Map.merge(
    project, 
    %{budget: budget, budget_used: budget_used, budget_remaining: budget_remaining},
    fn _, existing, _ -> existing end
  )
end

def get_project_with_budget!(id) do
  id
  |> get_project!()
  |> add_budget_data()
end

# in the controller module
def show(conn, %{"id" => id}) do
  project = Clients.get_project_with_budget!(id)
  changeset = Clients.change_budget_item(%BudgetItem{project_id: project.id})
  render(conn, "show.html", project: project, changeset: changeset)
end
1 Like
9

Map.merge/3 is nice succinct solution but I think he wishes to see how to use pipes to transform data. I’m sure he’ll revisit it once he gains more experience. It’s a little harder to understand what Map.merge does as opposed to Map.put

10

That’s fine. I think there is a practical reason not to create separate functions for each operation - you never want those functions to be called in isolation. You would never call just the function to update the budget and the budget used without also updating the budget remaining, because you would have values that don’t tie (i.e., budget - budget_used might not equal budget_remaining.)

So while normally I would say the degree to which you extract smaller functions from larger functions is ultimately a matter of personal style, in this case it is probably better not to make it possible to use any of these three update functions in isolation.

4 Likes
11

Good point you’d have to be careful not to call those functions in isolation. I’m not sure exactly what he’s trying to accomplish. Just gave an example of how to pipe it. Great input tho, something he should keep in mind.

2 Likes
12

In this specific case I would avoid using pipes as for example reduce on fields requires to write Map.put/3 only once and therefore allows to focus on working on data. Also if you are updating map/struct key value then you can simply use %{map | existing_key: new_value} syntax which is short and nice.

Here are 2 examples … First goes short update map:

project = Clients.get_project!(id)
budget = total_budget(project.budget_items)
budget_used = budget_used(project.work_items)
updated_project = %{project | budget: budget, budget_used, budget_remaining: budget - budget_used}

Second example shows usage of reduce:

defmodule Project do
  defstruct [:budget, :budget_items, :budget_remaining, :budget_used, :work_items]

  def calculate(project) do
    fields = [:budget, :budget_used, :budget_remaining]
    Enum.reduce(fields, project, fn key, acc ->
      value = Project.calculate(project, key)
      Map.put(project, key, value
    end)
    # short notation:
    # updated_project = Enum.reduce(fields, project, &Map.put(&2, &1, Project.calculate(&2, &1))
  end

  def calculate(%{budget_items: items}, :budget), do: total_budget(items)
  def calculate(%{work_items: items}, :budget_used), do: budget_used(items)
  def calculate(%{budget: budget, budget_used: used}, :budget_remaining), do: budget - used
end

project = id |> Clients.get_project!() |> Project.calculate()
1 Like
13

Doesn’t %{map | key: value} syntax overwrite existing keys? That would not match what the OP is doing (using Map.put_new/3).

2 Likes
14

Good ole reduce will do the job as well.

def show(conn, %{"id" => id}) do
  project = Clients.get_project!(id)

  total = total_budget(project.budget_items)
  used = budget_used(project.work_items)

  values = [
    budget: total,
    budget_used: used,
    budget_remaining: total - used
  ]

  project = Enum.reduce(values, project, fn {k, v}, project -> Map.put_new(project, k, v) end)

  changeset = Clients.change_budget_item(%BudgetItem{})
  render(conn, "show.html", project: project, changeset: changeset)
end
2 Likes
15

correct

Hmm … I’m not sure if assuming that only by it is good …

First of all it looks like a typical Phoenix app which is using Ecto.Schema (I guess that there are just virtual fields). The fields are generated only in the controller for show action. Of course there may be something in Clients.get_project!/1, but we can’t be sure about that. If that’s a typical phoenix context function then it’s just a simple query by id.

If we are using struct then put_new/1 would have no affect as project would have default value (or nil) already. We know that those fields are generated based on other fields (imagine full_name = "#{first_name} #{last_name}". In most cases such fields could be update (or more precisely re-generated) several times and therefore more interesting here is if related fields are changing.

Finally Map.put_new/3 is not good for such usage. Look that total_budget/1 and budget_used/1 functions would be called regardless if there would be already value or not - in many cases it causes many problems. When I see such mistakes I just can’t assume such things and I’m simply providing a solution for most typical cases.

The best way is to provide sample input and output right after question, so our examples could be tested properly. Right now we guess that x solution is good or not. That’s said op asked more about code organization than a typical solution. I think that my examples are helpful also in this part.

Regarding my solution it’s really easy to update it by just wrapping do … end block inside an if with simple condition using Map.has_key?/2 …

defmodule Project do
  defstruct [:budget, :budget_items, :budget_remaining, :budget_used, :work_items]

  def calculate(project) do
    fields = [:budget, :budget_used, :budget_remaining]
    Enum.reduce(fields, project, fn key, acc ->
      if Map.has_key?(project, key) do
        project
      else
        value = Project.calculate(project, key)
        Map.put(project, key, value
      end
    end)
    # short notation:
    # updated_project = Enum.reduce(fields, project, &Map.put(&2, &1, Project.calculate(&2, &1))
  end

  def calculate(%{budget_items: items}, :budget), do: total_budget(items)
  def calculate(%{work_items: items}, :budget_used), do: budget_used(items)
  def calculate(%{budget: budget, budget_used: used}, :budget_remaining), do: budget - used
end

project = id |> Clients.get_project!() |> Project.calculate()

Look that my solution is really easy to change.

3 Likes
16

Yep!

I think I am giving people the impression that I don’t think these alternatives are helpful. They obviously all are! I just thought it would be good to note when an alternative doesn’t actually do what the original code did.

I agree that everything about the original code snippet feels like, “I am getting an Ecto struct, doing some stuff to it in a Phoenix controller or context, and then sending it to a view/template.” It follows that using Map.put_new/3 will not have any effect on the struct being passed to it for exactly the reasons you note. We agree that trying to replicate such behavior via other means is not a worthy goal.

If project is an Ecto struct, I would just calculate the three budget values and use %StructName{map | key: value} syntax (exactly like your first example). You could also use Kernel.struct!/2, but I don’t know of any reason to prefer one over the other in this case.

2 Likes