Code review for a newbie

Hello everyone!

I’m fairly new to Elm and to functional programming in general. It’s something I’ve dabbled with in the past, but really haven’t spent much time working with it. I wrote a small toy program to familiarize myself with Elm. You can see it in action here: https://www.lakin.ca/avatar/ the source code can be found on github: https://github.com/lakinwecker/avatar.lakin.ca

While I’m generally happy with the results, I the resulting code is longer than I would have expected. I’ve done a couple passes of cleaning it up. Before I go further, I thought I would reach out and see if someone with more experience could review it and critique it. Because this is an exercise in improving my skills in both Elm and FP in general, please feel free to be critical in your review, I don’t mind. :smiley:

For anyone that does take the time to look at it, thank you very much!

This looks really good! Minor nitpicks

wrap : Int -> Int -> Int
wrap max val =
    case val < 0 of
        True ->
            wrap max (val + max)

        False ->
            case val >= max of
                True ->
                    wrap max (val - max)

                False ->
                    val

Usually, I prefer if-then-else for matching on booleans. In this specific case, it also seems possible to use modBy (typically written % in other languages).

type alias Index =
    ( Int, Int )


x : Index -> Int
x idx =
    Tuple.first idx


y : Index -> Int
y idx =
    Tuple.second idx

Why not make Index a record, e.g. type alias Index = { x : Int, y : Int }. Records are almost always a better idea: the fields are automatically documented and you get projection functions for free (the .x and .y in this case.

toIndices : Cells -> List Index
toIndices cells =
    List.map
        Tuple.first
    <|
        Dict.toList
            cells

I almost always prefer parens over <|, so

toIndices : Cells -> List Index
toIndices cells =
    List.map Tuple.first (Dict.toList cells)

I wonder if your feeling of having written a lot of code is because of pure linecount, or does it feel like you need more logic in elm? You’ve extracted many small functions which is easy in elm and looks nice, but you maybe wouldn’t have done that in other languages? Elm’s formatting also takes a lot of vertical space.

2 Likes

Thank you!

Regarding modBy, good point. I overlooked this function.

Regarding records instead of the Tuple. I was unclear on whether records could be used as the comparable in a Dict:

A dictionary mapping unique keys to values. The keys can be any comparable type. This includes Int, Float, Time, Char, String, and tuples or lists of comparable types.

Regarding <| vs parenthesis. I flip-flopped on this decision a few times. I agree that your version of toIndices is nicer.

Regarding length: Yes, it’s line count, but also the number of times I had to write out longer cases etc. I think I just overlooked or forgot about the if-then-else construct, and will switch the boolean cases to use it. For line count, I may try and write this in another language, like say React and see where I end up to see if my feelings about the line count are purely misguided - which they may be. I might end up with a similar line count in that version.

I meant that line count may not be the best metric. Elm definitions have a type signature, so that is one extra line per function. As mentioned formatting uses a lot of vertical space. There are also some things in your code that look fine, but use a lot of vertical space, for instance

isSpawning : Cell -> Bool
isSpawning c =
    case c.state of
        Spawning ->
            True

        _ ->
            False

Could be written as

isSpawning : Cell -> Bool
isSpawning c =
    c.state == Spawning

Third of the line count, but fundamentally the same thing in terms of code complexity.

1 Like

Very cool animation! A few thoughts as I go through the code.

I see that you import things like Maybe, List, etc.
Some of these things (the ones listed in https://package.elm-lang.org/packages/elm/core/latest/) are imported by default, so you don’t need to do it again.

Another thing that strikes me is that you have potentially conflicting sources of truth with the neighbours in the Cell dictionary, and the list of white and black elements positions.
Your model could look potentially end up looking like this (if you have a bug in your logic somewhere):

{ board: <empty dict>
, l : [(1, 1)]
, w : []
, ...
}
-- Or the other way around
{ board: Dict.fromList [(1, 1), {state: Alive, color: White, neighbours: 5 }]
, l : []
, w : []
, ...
}

In the first model, you have an empty board but you somehow have a white cell. In the second model, you have a non-empty board, but no white or black cells.
In both cases, something is off, and ideally, we would like that to not happen.
I suggest (re)watching this talk on “Making Impossible States Impossible” https://www.youtube.com/watch?v=IcgmSRJHu_8
I think you compute the neighbours to avoid doing it again later (I might be wrong, I haven’t delved in this enough), but It’s probably better if you only have board without the neighbours, or only l and w, or another similar data structure containing the information you really want.
You would then have to compute the neighbours on every update that affects the board, but you kind of already do that. This way you would only have a single source of truth, which should lead to less bugs (you can create a dictionary of neighbours, just don’t store it in the model), but also probably remove quite a lot of functions related to assigning neighbours.

In the aliveClass, I see that you add a default pattern for the alive case. This is fine at the moment, but if one day you change the type for Cell (add a new value for instance), then potentially this could lead to a bug. I would suggest replacing _ -> "alive" by Alive -> "alive", that way when you change Cell, the compiler will have your back.

The same comment applies to functions like isSpawning. It may be a bit more lengthy to write all the cases, but I generally try to specify every matching pattern instead of using _ ->, so that the compiler can help me out. Maybe I’ll need to write NewCellConstructorName -> False every time, but at least the decision will be conscious and I won’t have to think “Oh, was there be a location where I should have changed how the pattern match was done?”.

I agree with what @folkertdev said, "I almost always prefer parens over <|". I usually prefer using |> over parens or <| (personal preference, do what you want with it).

onlyAlive =
    List.foldl
        spawnToAlive
        clearedDead
        (List.map
            (\( idx, _ ) -> idx)
            (Dict.toList
                (Dict.filter (\idx c -> isSpawning c) clearedDead)
            )
        )

-- Becomes

onlyAlive =
    clearedDead
        |> Dict.filter (\idx c -> isSpawning c)
        |> Dict.toList
        |> List.map (\( idx, _ ) -> idx)
        |> List.foldl spawnToAlive clearedDead

-- Which I notice can probably be simplified to

onlyAlive =
    clearedDead
        |> Dict.filter (\idx c -> isSpawning c)
        |> Dict.keys
        |> List.foldl spawnToAlive clearedDead

Happy learning!

2 Likes

Hello,

Generally I think your code is pretty nice. Personally I think you’re over-complicating mapNeighbours:

mapNeighbour : (NeighbourCounts -> a) -> Cell -> a
mapNeighbour m cell =
    m cell.neighbours


totalNeighbors : Cell -> Int
totalNeighbors =
    mapNeighbour (\n -> n.white + n.black)

The only other place it is used is here:

indicesThatSpawnColor : Cells -> (Int -> Int -> Bool) -> List Index
indicesThatSpawnColor board colorMatch =
...
               let
                    blackN =
                        mapNeighbour .black cell

                    whiteN =
                        mapNeighbour .white cell
                in
                    (not (isAlive cell)) && (blackN + whiteN) == 3 && (colorMatch whiteN blackN)

I would just directly use .neighbours, the good thing with a strongly typed language is that if you later change your mind about your data type and actually require some kind of mapping function the compiler will force you to change everywhere. So, to me, totalNeighbours is clearer as simply:

totalNeighbors : Cell -> Int
totalNeighbors cell =
    cell.neighbours.white + cell.neighbours.black

similarly:

indicesThatSpawnColor : Cells -> (Int -> Int -> Bool) -> List Index
indicesThatSpawnColor board colorMatch =
...
               let
                    blackN =
                        cell.neighbours.black

                    whiteN =
                        cell.neighbours.white
                in
                    (not (isAlive cell)) && (blackN + whiteN) == 3 && (colorMatch whiteN blackN)

A couple of other tiny points, here:

                let
                    n =
                        totalNeighbors cell
                in
                    (isAlive cell && (n < 2 || n > 3)) || (isDying cell)

I would give the middle condition a name shouldDie or similar, you could even take it out so that you could then re-write it as:

shouldDie : Cell -> Bool
shouldDie cell =
    let
          neighbours =
             totalNeighbours cell
    in
    neighbours < 2 || neighbours > 3

indicesThatShouldDie : Cells -> List Index
indicesThatShouldDie board =
    let
        shouldBeDying _ cell =
             (isAlive cell && shouldDie cell) || isDying cell
    in
    board
        |> Dict.filter shouldBeDying
        |> toIndicies

Not sure why, but I think this makes the condition really clear, it’s all about the current state of the cell, the fact that you’re doing data structure manipulations is kind of secondary.

Finally:

changeCell : Color -> State -> ( Int, Int ) -> Index -> Cells -> Cells
changeCell color state i idx board =

Here you use an updateNeighbour function. Instead I would just figure out what the neighbours are, and then completely re-calculate each neighbour’s new state. That would mean you could get rid of modifyNeighbourCount.

2 Likes

You’re right, they cannot be used as keys, so in this situation using a type alias is probably the best choice. If you want more type safety, I think the only real way is to create a module that provides a special IndexDict using an custom type Index in all public functions and translates those to tuples internally. This can have benefits but I think your solution is fine for what you want to do.

1 Like

Understood that it’s the same in terms of complexity, but I prefer the second. dunno why I wrote them all that way.

Thank you for all the replies. I’ve learned a lot and I really appreciate it. :heart:

2 Likes

Final update from me (unless I get more feedback).

I’ve updated the source to be more in line with all of your suggestions with the exception of trying to make impossible states impossible. I fully agree that this is a great goal, but in this case I’m not too fussed about it, because this was more about learning the elm syntax and patterns. I’ll probably take a shot at that at some point, but don’t have the time right now.

In regards to my comment about the code feeling too “long”: after all of the feedback I am feeling much better about the code. Yes, lines of code are larger than I was expecting, but I don’t mind it with the new version. The new version is much more clear and succinct than the initial version I posted an I’m very happy with the results.

Thanks again for everyone for responding. :smiley:

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