How to refactor this conditional piping

Request Feedback
1
  1. This code doesn’t seem idiomatic, where the piping is based on the inter boolean value. How can it be improved?
  2. Not that it is important, but is it possible to refactor and avoid the List.Extra function and still get the same effect?
calculateDiff : List ( Int, Int ) -> Bool -> List ( Int, Int )
calculateDiff tlst inter =
    let
        ( _, lst ) =
            List.unzip tlst

        ziplist =
            List.map2 Tuple.pair lst (withDefault [] (List.tail lst))
                |> List.map (\( x, y ) -> y - x)

        lst3 =
            ziplist |> List.Extra.groupsOf 3 |> List.map List.sum
    in
    List.indexedMap Tuple.pair
        (if inter then
            ziplist

         else
            lst3
        )
2

I think your problems come from the fact that this function does a lot of things on different levels:

  1. It throws away the first component of each entry. (This seems like data preparation and should maybe be part of some parsing step earlier?)
  2. It contains an ad-hoc implementation of function to map over adjacent pairs in a list. (This should be its own function.)
  3. It calculates the pairwise differences. (This is what you want, I think.)
  4. It contains logic to decide whether to calculate sums of groups of three. (This seems like an attempt to roll two different functions into one. Maybe it is also a choice based on some user-interface switch? I don’t know the surrounding code.)
  5. It adds first components again. (This seems superfluous.)

I would probably go for something like

mapAdjacent : (a -> a -> b) -> List a -> List b
mapAdjacent f list =
    List.map2 f list (withDefault [] (List.tail list))

calculateDiff : List Int -> List Int
calculateDiff = mapAdjacent (\x y -> y - x)

calculateDiff3 : List Int -> List Int
calculateDiff3 = calculateDiff >> List.Extra.groupsOf 3 >> List.map List.sum

and then try to remove the first components earlier, i.e. at the call site. By the way, I think List.map Tuple.second tlst is the more elegant way to do that. I would also not add the indices to the list because they are implicitly contained in the order of the list. If you need them later, you can use List.indexedMap where you actually need them. Lastly, I would also move the if that decides whether to use calculateDiff or calculateDiff3 to the call site.

4 Likes
3

You have amazingly read my mind! And I think I understand all your suggestions!
Thanks much!

As for the use of List.Extra.groupsOf function, is that reasonable?

4

You’re welcome. If what you want is to add groups of three (i.e. turn [1,2,3,4,5,6,7] into [1+2+3, 4+5+6]) then List.Extra.groupsOf is the way to go, I think. Note that this function just discards elements at the end if there are not enough for a final group. If you want something else, there is also List.Extra.greedyGroupsOf or you might have to roll your own.

1 Like
5

The refactor worked perfectly!

List.Extra.groupsOf is perfect for my use case, so I will stick with that.

In my calling site, I now have this (lst being the output of calculateDiff or calculateDiff3):

   lst
        |> List.indexedMap Tuple.pair 
        |> List.map (\t -> ( t, String.fromInt (value t) ++ ", " ))
        |> --- ...further piping
  1. Can the indexedMap and map be refactored to a single piping sequence?|
  2. What is the meaning of the annotation (a -> a -> b) as found in mapAdjacent?
6

You can do

lst
    |> List.indexedMap (\index val -> ( (index, val), String.fromInt (value (index, val)) ++ ", " ))

where you can of course change value (index, val) to simply val if the function value just extracts the second component again. By the way, if value does not use the index, I would argue that your use of indexedMap is still to early. I would put it right before the point where I actually need the index.

The (a -> a -> b) in the type of mapAdjacent means that the first parameter is a function that takes two values of the same type a and returns a value of a (potentially, but not necessarily) different type b. In your case, both a and b are simply Int because the function (\x y -> y - x) turns two Ints into one Int.

1 Like
7

Is it (\index val ->

or do I need a comma in between index and val?
Sorry, my bad. It did compile without the ,.

When does one use a comma and when is it not required?

8

If I’m not forgetting something, commas are only used in Elm:

  • between list items, [1, 2, 3]
  • between tuple items: (1, 2, 3)
  • between record fields: {firstname = "foo", lastname = "bar"}
  • when exposing symbols: import Html exposing (Html, div, text)

That’s all. Never use a comma else, including between functions or lambdas arguments.

1 Like
9

Between

and

calculateDiff3 lst = 
   lst |> calculateDiff |> List.Extra.groupsOf3 |> List.map List.sum

which is preferable? And why?

10

Both of them do the same thing. I like that the first one is more concise and that I don’t have to think of a name for the variable. But you might find the second one more readable and then you should probably go with that one.

11

This topic was automatically closed 10 days after the last reply. New replies are no longer allowed.