2
\$\begingroup\$

I'd like to learn if my rewrite is worse than the original, less clear or less "idiomatic".

I've found this code that handles updating two dimensional matrixes in Elm:

type alias Matrix a = Array.Array (Array.Array a)
type alias Location = (Int, Int)

get : Location -> Matrix a -> Maybe a
get location m =
  Array.get (row location) m `andThen` Array.get (col location)


set : Location -> a -> Matrix a -> Matrix a
set location value m =
  update location (always value) m

update : Location -> (a -> a) -> Matrix a -> Matrix a
update location f m =
  get location m
  |> Maybe.map (
    \current ->

      Array.get (row location) m
      |> Maybe.map (
          \oldRow ->
            Array.set (col location) (f current) oldRow
            |> (\newRow -> Array.set (row location) newRow m)
      )
      |> Maybe.withDefault m
  )
  |> Maybe.withDefault m

I couldn't understand update at first so I tried to write my own (using my own domain types):

type Player = PlayerOne | PlayerTwo
type Slot = Empty | PlayedBy Player
type alias Board = Array.Array (Array.Array (Slot))
type alias Position = (Int, Int)

get : Board -> Position -> Maybe Slot
get board (row, column) =
    Array.get row board `Maybe.andThen` Array.get column

setPosition : Board -> Position -> Player -> Board
setPosition board (row, column) player = 
    case get board (row, column) of
        Nothing ->
            board
        Just Empty ->
            case Array.get row board of
                Nothing ->
                    board
                Just oldRow ->
                    let
                        newRow = Array.set column (PlayedBy player) oldRow
                    in
                        Array.set row newRow board
        Just s ->
            board

After rewriting I could understand what's going on in the original update function, but ended up feeling my version's clearer. This is my first day writing Elm so I'm no authority in Elm code clarity.

\$\endgroup\$
  • 1
    \$\begingroup\$ Your code looks very clear to me. In a way, first-day programmers are the best authority in clarity :) Maybe you could write an applyAt : Int -> (a -> a) -> Array a -> Array a function, and see if your code is clearer if you use that twice? \$\endgroup\$ – Lynn Mar 17 '16 at 18:31
4
\$\begingroup\$

I like your code a lot more than the original! It is definitely cleaner and more obvious than the first example you gave. However, there still seems to be a process you’re repeating — get value from array, pattern match on it, update array — that you can still factor out. (The original code is guilty of this, too.)

I tried to rewrite your function in a matter that avoids this, and my code’s at the bottom; see if you find it more readable. But first, some stylistic advice on writing Elm:

  • Generally, instead of writing things like Array.Array, it's useful to expose the type name from the data structure package you're using. Also, type parameters don’t need parentheses around them (except for grouping).

    So you can rewrite:

    type alias Board = Array.Array (Array.Array (Slot))
    

    to

    import Array exposing (Array)
    
    type alias Board = Array (Array Slot)
    

    which I think is a bit more Elm-y.

  • A useful design guideline (for packages, but it’s a good idea for code in general): the data structure is always the last argument. You’ve reordered them from your first example, but the original order was there for a good reason: if your types look like

    f : (extra params) -> Container -> Container
    

    you can partially apply your function and end up with:

    f "extra" "arguments" : Container -> Container
    

    which is very useful for function composition and piping: often you want to take a Container and pipe it through a bunch of Container -> Container functions, like so:

    result =
        initialContainerValue
        |> f "hello" "world"
        |> someOtherStep
        |> f "oak" "fir"
    
  • The Elm style guide contains some more recommendations that could apply to your code, but they’re more subjective. The two big ones I could think of have to do with whitespace.

    • Always have 2 empty lines between top-level declarations.
    • The examples in the guide have single empty lines between case branches, too.

My version

{-| Apply a function at the given index in the array.

     applyAt 2 negate (fromList [50, 60, 70]) == fromList [50, 60, -70]
-}
applyAt : Int -> (a -> a) -> Array a -> Array a
applyAt i f array =
    case Array.get i array of
        Nothing ->
            array

        Just a ->
            Array.set i (f a) array


{-| Update a slot: the given player occupies it, but only if it's empty. -}
setSlot : Player -> Slot -> Slot
setSlot player slot =
    case slot of
        Empty ->
            PlayedBy player

        s ->
            s


{-| Update a slot somewhere in a board. -}
setPosition : Position -> Player -> Board -> Board
setPosition (row, column) player =
    applyAt row
        (applyAt column
            (setSlot player))
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for pointing me to the design guideline and explaining the argument order, I'm used to Elixir piping and thought the data structure should be first =] \$\endgroup\$ – Juliano Mar 17 '16 at 21:55
  • \$\begingroup\$ setPosition is waaay clearer now, I see your point! \$\endgroup\$ – Juliano Mar 17 '16 at 21:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.