How best to write this function

Questions / Help
1

I have the following function which I’m struggling with a bit. What it has to do is check for the existence of the following values as a comma-separated string, then convert that string to a list. So, simple enough. I’m just struggling a bit with how to write it in a correct form in elixir. Below is what I have, which runs, but it rightly raises a compile warning.

Any help in how to better structure this is much appreciated :slight_smile:

  defp convert_outcodes(changeset) do
    zone_a_string = get_change(changeset, :zone_a_outcode_string)
    zone_b_string = get_change(changeset, :zone_b_outcode_string)

    if zone_a_string do
      split = zone_a_string |> String.split(",")
      changeset = put_change(changeset, :zone_a_outcodes, split)
    end
    if zone_b_string do
      split = zone_b_string |> String.split(",")
      changeset = put_change(changeset, :zone_b_outcodes, split)
    end
    changeset
  end
2 Likes
2

It’s untested, but you could try something like this.

defp convert_outcodes(changeset) do
   changeset
   |> process_zone_string(:zone_a_outcode_string)
   |> process_zone_string(:zone_b_outcode_string)
end

defp process_zone_string(changeset, zone_outcode_string) do
  get_change(changeset, zone_outcode_string)
  |> split_string(zone_outcode_string, changeset)
  |> commit_change(split_string, zone_outcode_string, changeset)
end

defp split_string(nil, zone_outcode_string, changeset), do: commit_change(nil, zone_outcode_string, changeset)
defp split_string(change, zone_outcode_string, changeset), do: change |> String.split(",")

defp commit_change(nil, _zone_outcode_string, changeset) do: changeset
defp commit_change(split_string, :zone_a_outcode_string, changeset) do: put_change(changeset, :zone_a_outcodes, split_string)
defp commit_change(split_string, :zone_b_outcode_string, changeset) do: put_change(changeset, :zone_b_outcodes, split_string)
5 Likes
3

I’d do something like (untested):

def convert_outcodes(changeset) do
  changeset
  |> split_outcode(:zone_a_outcode_string, :zone_a_outcode)
  |> split_outcode(:zone_b_outcode_string, :zone_b_outcode)
end

defp split_outcode(changeset, from_atom, to_atom) do
  case get_change(changeset, from_atom) do
    nil -> changeset
    change ->
      split = String.split(change, ",")
      put_change(changeset, to_atom, split)
  end
end
4 Likes
4

Here is my take. Untested and updated.

# somewhere in your code
zone_codes = [zone_a_outcode_string: :zone_a_outcodes,
              zone_b_outcode_string: :zone_b_outcodes] 

defp convert_outcodes(changeset, zone_codes) do
  Enum.reduce(zone_codes, changeset, fn {string_key, codes_key}, cset -> 
    string = get_change(cset, string_key)
    if string do
      put_change(cset, codes_key, String.split(string, ","))
    else
      cset
    end
  end)
end

and if you didn’t need the :zone_X_outcode string after processing (maybe) you could even do it with recursion and pattern matching by removing that change.

1 Like
5

The reason why is that your if’s do not have an else block, you must always have an else block. A fine way to do it (although breaking it up in to more functions is probably better) is just to fix that, then it should be fine:

  defp convert_outcodes(changeset) do
    zone_a_string = get_change(changeset, :zone_a_outcode_string)
    zone_b_string = get_change(changeset, :zone_b_outcode_string)

    changeset =
      if zone_a_string do
        split = zone_a_string |> String.split(",")
        put_change(changeset, :zone_a_outcodes, split)
      else
        changeset
      end

    changeset =
      if zone_b_string do
        split = zone_b_string |> String.split(",")
        put_change(changeset, :zone_b_outcodes, split)
      else
        changeset
      end
    changeset
  end

But still, more functions is better, especially with how repeatable that code is:

defp convert_outcodes(changeset) do
  changeset
  |> convert_outcodes_change(:zone_a_outcode_string, :zone_a_outcodes)
  |> convert_outcodes_change(:zone_b_outcode_string, :zone_b_outcodes)
end

defp convert_outcodes_change(changeset, in_tag, out_tag) do
  case get_change(changeset, in_tag) do
    nil -> changeset
    string ->
      split = String.split(string, ",")
      put_change(changeset, out_tag, split)
  end
end

Or something like that. (EDIT: Which is basically what some of the above did ^.^; )

2 Likes
6

Great minds think alike! :stuck_out_tongue:

2 Likes
7

Even with the else block it still raises scope warnings in Elixir 1.4 due to the (re)definition of the var inside the if block.

Went with the pattern matching option. Thanks @jeromedoyle.

1 Like
8

Er yes, split needs to be two different names in each one, Elixir has this really annoying scope leakage, just uniquely name variables and all is good. ^.^

1 Like
9

Just reviewed your change again, I see what you mean now. Thanks for replying.

Here’s my final version:

  defp convert_outcodes(changeset) do
     changeset
     |> process_zone_string(:zone_a_outcode_string)
     |> process_zone_string(:zone_b_outcode_string)
  end

  defp process_zone_string(changeset, zone_outcode_string) do
    get_change(changeset, zone_outcode_string)
    |> split_string
    |> commit_change(zone_outcode_string, changeset)
  end

  defp split_string(nil), do: []
  defp split_string(change), do: change |> String.split(",")

  defp commit_change(split_string, :zone_a_outcode_string, changeset), do: put_change(changeset, :zone_a_outcodes, split_string)
  defp commit_change(split_string, :zone_b_outcode_string, changeset), do: put_change(changeset, :zone_b_outcodes, split_string)
2 Likes
10

Just a small observation. According to the style guide you don’t pipe in just one function. So you maybe prefer this

defp split_string(change), do: String.split(change, ",")

over this

defp split_string(change), do: change |> String.split(",")

2 Likes