Elegant refactory of a function

I’ve got this function, translated from an old codebase (imperative language) into Elixir, by our contractor.

  def pub0(payload) when is_map(payload) do
    if ["country", "commodity", "share", "mon", "cog", "esa1"] |> Enum.all?(fn x -> Map.has_key?(payload, x) end) &&
        Map.get(payload, "esa1") == "hs10" &&
        is_intger(Map.get(payload, "cog") do
      payload = Map.put(payload, "itime", utimestamp(2))

The function works fine, it performs its job, but I don’t like the code structure:

There is an if with tree conditions that have to evaluate all to true (logically: if (true & true & true) then …). If satisfices this, then adds new property to the object and finally calls another function.

It does not seem functional. Is there a way to refactory the function in a (more) elegant and functional manner? And also more comprehensible, consistent, shorter ?

Thanks! :smiley:

Personally, I’d probably go for this:

def pub0(%{"country" => _, "commodity" => _, "share" => _, "mon" => _, "cog" => cog, "esa1" => "hs10"} = payload) when is_integer(cog) do
    |> Map.put("itime", utimestamp(2))
    |> Jason.encode!
    |> emporio_ts()
def pub0(_), do: nil

The usual micro-scale cleanups could help (extract the parts of that giant conditional into functions with meaningful names, for instance) but the shape-checking on payload makes me wonder if it should be a struct.

That would require much larger changes to the calling code, but simplify pub0 considerably.

This looks very cool, but it fails: (MatchError) no match of right hand side value.
Maybe because the payload has much more properties than those six tested for. The rest are not mandatory but in total are ~30 properties.
Actually I learnt that (in prod) the number of props of the payload is variable, between 20 and 35.

No, maps (thankfully) support partial matches :slight_smile:

Share your code and the exact map you’re passing to pub0, I’m sure we can figure out what’s going on

I’ve learnt map support partial matches but still it fails with that error.
Unfortunately, I can’t share more code/data (for legal reasons), I have to debug it myself.

1 Like

It can’t fall with that catch-all returning nil …?

1 Like