Can you help me improve my program (Tic-Tac-Toe)?

Request Feedback
1

Hi everyone,
I mentor high-schoolers doing Elm programming and I recently walked a mentee through the implementation of a Tic-Tac-Toe. This was super fun, and I would love to have some feedback on the code we produced :pray:

Here is the code (~250 lines), can you please have a look?
https://ellie-app.com/g7zD3zmTvjRa1

Before teaching Elm, I developped Python professionally and although I am comfortable with functional programming, some things still felt awkward for this project:
The use of case expression actually induced some repetition and unnecessary code, as you must provide an expression for patterns you would just have omitted in a more imperative language.

For example:

  • returning an empty display group line 183
  • several mention of retryView lines 216, 228, 237, when only the other element changed.

If there you think of a technique for this specific issue, I’ll be glad. :slight_smile:

Thanks in advance :v:

2 Likes
2

I guess it’s a matter of taste but you can refactor case ... of ... usually into Maybe.map together with Maybe.withDefault (similar functions exists for Result and a bunch of other types).

Personally I prefer this style but might want to introduce helper functions (for the Maybe.map action) if you don’t like more nesting.

Here it is for your example 183:

cellView : Maybe Player -> Int -> Cell -> Shape Msg
cellView winner index cell =
    let
        cellPositions =
            Array.fromList [ ( -100, 100 ), ( 0, 100 ), ( 100, 100 ), ( -100, 0 ), ( 0, 0 ), ( 100, 0 ), ( -100, -100 ), ( 0, -100 ), ( 100, -100 ) ]
    in
    cellPositions
    |> Array.get index
    |> Maybe.map (\position ->
        cell
        |> Maybe.map (\_ ->
            group
              [ squareView position
              , circleView position
              ])
        |> Maybe.withDefault (
            case winner of
                Nothing ->
                    squareView position 
                    |> notifyTap (ClickMsg index)
                _ ->
                    squareView position))
    |> Maybe.withDefault (group [])
3

you can refactor this in:

endGameView : String -> Shape Msg
endGameView endText =
    group
        [ text endText
            |> centered
            |> filled lightBlue
            |> scale 3
            |> move ( 0, 175 )
        , retryView
        ]


view : Model -> Collage Msg
view model =
    let
        winner =
            checkWin model.grid
    in
    collage 500
        500
        [ gridView model.grid winner
        , case winner of
            Nothing ->
                if checkGridFull model.grid then
                    endGameView "Draw"

                else
                    group []

            Just Circle ->
                endGameView "Circle Wins!"

            Just Cross ->
                endGameView "Cross Wins!"
        ]
4

Here is an alternative implementation of that section:


cellPositions =
    [ ( -100, 100 ), ( 0, 100 ), ( 100, 100 ), ( -100, 0 ), ( 0, 0 ), ( 100, 0 ), ( -100, -100 ), ( 0, -100 ), ( 100, -100 ) ]


cellView : Maybe Player -> Int -> ( ( Float, Float ), Cell ) -> Shape Msg
cellView winner index ( position, cell ) =
    case cell of
        Just Circle ->
            group
                [ squareView position
                , circleView position
                ]

        Just Cross ->
            group
                [ squareView position
                , crossView position
                ]

        Nothing ->
            case winner of
                Nothing ->
                    squareView position |> notifyTap (ClickMsg index)

                _ ->
                    squareView position


gridView grid winner =
    Array.toList grid
        |> List.map2 Tuple.pair cellPositions
        |> List.indexedMap (cellView winner)
        |> group
5

Yes I see how this is an improvement, although I am sad about loosing a nicely curried function, haha.

Good point, thanks! I had overlooked the centered funcion, it sure makes refactoring easier.

A nice addition to my toolbox, thanks :slight_smile:

This is very instructive, it seems piping does improve readability a lot. Is it widely used? Both |> and <|?

6

Forward piping (|>) is used very heavily in idiomatic Elm code. Backpiping (<|) not so much, although it does have it’s uses (particularly in elm-test).

1 Like
7

As I read recently here on discourse, it is important to be aware of the fact that withDefault argument always gets evaluated, so it is good for values but not for function calls:

    case maybeV of
        Just v ->
            v

        Nothing ->
            createNewV randomSeed -- complex calculation that takes very long.


    maybeV
        |> Maybe.withDefault (createV randomSeed) -- the calculation will be performed always even when it is not necessary.

I guess that you can create a lazy version of withDefault:


withLazyDefault : (() -> a) -> Maybe a -> a
withLazyDefault fallbackFn mV =
    case mV of
        Just v ->
            v

        Nothing ->
            fallbackFn ()

resolvedV =
    maybeV
        |> withLazyDefault (\() -> createV randomSeed) -- only executed when needed.
8

Despite what @francescortiz is true, in this context I don’t think it really matters. I built a 50k LoC project without bothering with this kind of things and the app doesn’t have any performance issue… Don’t forget that:

Premature optimization is the root of (almost) all evil in programming

9

For the record, I settled on the following to draw the grid: the grid is responsible to place the cells so I can recover a curried function to draw the cell. Thanks @pdamoc for the inspiration!

cellView : Maybe Player -> Int -> Cell -> Shape Msg
cellView winner index cell =
    case cell of
        Just Circle ->
            group
                [ squareView
                , circleView
                ]

        Just Cross ->
            group
                [ squareView
                , crossView
                ]

        Nothing ->
            case winner of
                Nothing ->
                    squareView |> notifyTap (ClickMsg index)

                _ ->
                    squareView


gridView grid winner =
    let
        cellPositions =
            [ ( -100, 100 ), ( 0, 100 ), ( 100, 100 ), ( -100, 0 ), ( 0, 0 ), ( 100, 0 ), ( -100, -100 ), ( 0, -100 ), ( 100, -100 ) ]
    in
    Array.toList grid
        |> List.indexedMap (cellView winner)
        |> List.map2 move cellPositions
        |> group

I still dislike how my code relies on convention for cell placement and victory condition, but I guess I should have modeled the grid as type alias Grid = Dict (Int, Int) Player or something.

I am quite happy with the improvements made, so thanks to everyone who chimed in :smiley:
https://ellie-app.com/g9m7cHC8mDfa1

1 Like
10

You could use: elm-vector 3.0.2 to represent your board (since you only need 9-items vector , you could even write it yourself). That way, you can enforce that a cell coordinate is always “valid”.

Also, you could change your Model type to be:

type Model
    = GameRunning { board: Board, currentPlayer: Player }
    | GameEnded { winner : Player }

so it’s crystal clear what you have to display !

11

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