ARRG! Still stuck in the imperative world! Help! :)

Have a Map whose values are MapSets: Best way to update?

Currently have:

keys
|> Enum.reduce(
  %{},
  fn key, acc ->
    acc
    |> Map.put(
      key,
      Map.get(acc, key, MapSet.new())
      |> MapSet.put(get_value_to_add(key))
    )
  end
)

It is not very clear what You want to achieve… better put some data and expected result.

Anyway, this looks wrong (bad parens placement…)

Map.get(acc, key, MapSet.new())
      |> MapSet.put(get_value_to_add(key))

and piping once is something I try not to do. At least two pipes, otherwise I pass the parameter normally.

There is a Map.update function :slight_smile:

2 Likes

Could alternatively be spelled:

update_in acc, [Access.key(key, MapSet.new())], &MapSet.put(&1, get_value_to_add(key))
3 Likes

better way for updating the MapSet within the Map?

["apple", "apricot", "banana", "blueberry", "kiwi", "blackberry"]
|> Enum.reduce(
  %{},
  fn word, acc ->
    first_letter = String.at(word, 0)
    acc |> Map.put(
      first_letter,
      Map.get(acc, first_letter, MapSet.new()) |> MapSet.put(word)
    )
  end
)
|> inspect
|> IO.puts
$ elixir ~/Downloads/elixir-tmp-1.exs
%{"a" => #MapSet<["apple", "apricot"]>, "b" => #MapSet<["banana", "blackberry", "blueberry"]>, "k" => #MapSet<["kiwi"]>}

Except the MapSet…

iex> list = ["apple", "apricot", "banana", "blueberry", "kiwi", "blackberry"]
["apple", "apricot", "banana", "blueberry", "kiwi", "blackberry"]
iex> Enum.group_by(list, &String.first/1)
%{
  "a" => ["apple", "apricot"],
  "b" => ["banana", "blueberry", "blackberry"],
  "k" => ["kiwi"]
} 

With MapSet…

list 
|> Enum.group_by(&String.first/1) 
|> Enum.reduce(%{}, fn {k, v}, acc -> Map.put(acc, k, MapSet.new(v)) end)
%{
  "a" => #MapSet<["apple", "apricot"]>,
  "b" => #MapSet<["banana", "blackberry", "blueberry"]>,
  "k" => #MapSet<["kiwi"]>
}

With MapSet, short version :slight_smile:

iex> list 
|> Enum.group_by(&String.first/1) 
|> Enum.reduce(%{}, &Map.put(&2, elem(&1, 0), MapSet.new(elem(&1, 1))))  
%{
  "a" => #MapSet<["apple", "apricot"]>,
  "b" => #MapSet<["banana", "blackberry", "blueberry"]>,
  "k" => #MapSet<["kiwi"]>
}
2 Likes

A matter of personal preference, but I don’t like that reduce, because I tend to avoid using reduces unless it’s necessary, Enum.into is you much much more readable friend.

list = ["apple", "apricot", "banana", "blueberry", "kiwi", "blackberry"]

list
|> Enum.group_by(&String.first/1)
|> Enum.map(fn 
  {key, value} -> {key, Enum.into(value, MapSet.new()}
end)
|> Enum.into(%{})

I always think reduce is better than map into… but last time I checked with benchee, I was proved wrong :slight_smile:

1 Like

I tend to do things where network is very much the bottleneck, and correctness and easy debug/read legacy code is more important, so I’ve slowly let go of caring about performance =D

My only readability concern with my code is that it’s not immediately obvious that “group_by” returns a map. so I might put a comment to that effect. Also, I’m really surprised there wasn’t a List.to_mapset…

1 Like

At least here I prefer … MapSet.new(value) :slight_smile:

4 Likes

oh wow, great point. Yeah. Do that instead of my nested Enum.into monstrosity. I think I didn’t think of that because it “feels too OO”, lol.

1 Like

That applies here too.

You could use…

|> Map.new()

…instead

4 Likes

There’s a limit to how much I’m willing to let my code look like it’s object oriented :sweat_smile:

3 Likes

i certainly agree with this sentiment! :laughing:

so, here is my code now: for a list of process names, get their pids from ‘tasklist’.

i love the group_by solution, seems exactly right! this is the problem with being an old imperative/python programmer trying to get the heck out! :stuck_out_tongue_winking_eye:

note, this is everything in-lined so its a bit ugly but, any suggestions on how to be more functional and/or elixir-y please!

  def get_pids_for_names2(proc_names) do
    proc_names = MapSet.new(proc_names)

    System.cmd("tasklist", [])
    |> (fn {output, 0} -> output end).()
    |> String.split("\r\n") |> Enum.drop(3)
    |> Enum.reject(&(String.trim(&1) == ""))
    |> Enum.map(&String.split/1)
    |> Enum.filter(fn [name|_] -> MapSet.member?(proc_names, name) end)
    [|> Enum.map(fn [name,pid|_] -> [name, pid] end)] <-- not needed, removed in a later edit
    |> Enum.group_by(
         fn [name|_] -> name end,
         fn [_, pid|_] -> pid end
    )
  end

thoughts?

1 Like

Please use pipe for code formatting consistency:

"tasklist"
|> System.cmd([])

For more information take a look at pipeline operator in Elixir Style Guide by @lexmag


Avoid using anonymous functions in pipe.

# shorter version:
|> elem(0)
# gets first element of `Tuple`

# longer version (more readable):
|> success_command_output()
# …

defp success_command_output({output, 0}), do: output

For more information pleast take a look at:

  1. Kernel.elem/2 documentation
  2. Don’t use anonymous functions in pipelines in Elixir Style Guide

Just a quote:

For a multi-line pipeline, place each function call on a new line, and retain the level of indentation.

Example:

    |> String.split("\r\n")
    |> Enum.drop(3)

Source: https://github.com/lexmag/elixir-style-guide#pipeline-operator


For comments please use # comment syntax.

# …
# not needed, removed in a later edit
# |> Enum.map(fn [name,pid|_] -> [name, pid] end)
# …

In this case you may want to use List.first/1 and/or Enum.at/3:

# shorter version:
    |> Enum.group_by(&List.first/1, &Enum.at(&1, 1))

# longer version (more readable):
  |> Enum.group_by(&get_name_from_list/1, &get_pid_from_list/1)
# …
  defp get_name_from_list(list) when is_list(list), do: List.first(list)

  defp get_pid_from_list(list) when is_list(list), do: Enum.at(list, 1)

Note: In case you are worried about unexpected nil values you can use Enum.fetch!/2 in first case and pattern match inside function clause in second case.

For more information please take a look at:

  1. Enum.at/3 documentation
  2. Enum.fetch!/2 documentation
  3. List.first/1 documentation
1 Like