For the most part I think your Elm is entirely fine, I’d be fairly happy receiving this sort of code and being asked to work on it. It’s the nature of feedback that it’s mostly negative so bear in mind that I think generally this code is of pretty good quality.
Your source code comments are mostly useless. You have a lot of this kind of thing:
--
-- MAIN
--
main : Program () Model Msg
even worse:
--
-- SUBSCRIPTIONS
-- (new frames & new user inputs)
--
subscriptions _ =
Sub.batch <|
[ Browser.Events.onKeyDown (Decode.map KeyPushed keyDecoder)
, Browser.Events.onAnimationFrameDelta Frame
]
Note, if you really think this requires documentation, how about:
subscriptions _ =
let
userInput =
Browser.Events.onKeyDown (Decode.map KeyPushed keyDecoder)
newFrames =
Browser.Events.onAnimationFrameDelta Frame
in
Sub.batch <|
[ userInput
, newFrames
]
All I’m doing here is translating your comments into executable code that will get checked by the compiler.
You might think the following is helpful:
--
-- UTILS
--
asSnakeIn : Model -> Snake -> Model
asSnakeIn model snake =
{ model | snake = snake }
I don’t think I’ve ever once thought “I wonder where all the utility functions are?” in code that has omitted such a comment. I’m usually looking for a particular function and I can just do that by searching for it. If you really think that these functions deserve to be grouped together somehow, how about actually using the module system? In this case, I can see why you wouldn’t because making a ‘Utils’ module would necessitate also factoring out some of the types. This is also a pretty small project so to me it feels right that it’s all in a single file. This sort of situation makes me pine for syntactic modules that do not necessarily correspond to a file, but that’s digressing somewhat. The point is your UTILS
comment is more distracting than useful.
Here’s another one that looks useful:
{- Making sure the player cannot reverse the direction.
-}
canUpdateDirection : Direction -> List Direction -> Bool
canUpdateDirection newDirection directions =
Just rename the function preventDirectionReverse
or have that as a let
introduced condition:
canUpdateDirection : Direction -> List Direction -> Bool
canUpdateDirection newDirection directions =
let
latestDirection =
List.reverse directions |> List.head |> Maybe.withDefault LEFT
isDirectionReverse =
(newDirection == UP && latestDirection == DOWN)
|| (newDirection == DOWN && latestDirection == UP)
|| (newDirection == LEFT && latestDirection == RIGHT)
|| (newDirection == RIGHT && latestDirection == LEFT)
in
(newDirection /= latestDirection)
&& not isDirectionReverse
Note I might write that as:
isDirectionReverse =
List.member
(newDirection, latestDirection )
[ (UP, DOWN)
, (DOWN, UP)
, (LEFT, RIGHT)
, (RIGHT, LEFT)
]
but whatever your preference is.
Here’s another where a simple re-naming is better than a comment:
{- Model when starting OR re-starting the game
-}
getDefaultModel : Game -> Model
getDefaultModel game =
Just rename this to resetGame
or perhaps resetModelForGameStart
.
Actual code issue
So personally I feel there is really only one deficiency in the code, apples store whether or not they are ‘eaten’. But that seems to be in order to remove them, rather than because you want eaten apples to still be on your model. You betray this with your comment here:
type alias Apple =
{ position : Position
, eaten : Bool -- once eaten, the apple will be replaced
}
and similarly here (again the comment betraying the opportunity to refactor):
-- apples
-- (non-sense positions - replaced via the commands)
, apples =
[ { position = Position 49 49, eaten = True }
, { position = Position 50 50, eaten = True }
]
So first of all, I would just remove the apple from the list of apples when it is eaten. Secondly if you must use a command to generate the random new position then when that command messages back (in the handler to NewApplePosition
) just create the apple at that point, and add the newly created apply to the model. That means in your initial model you do not need the two ‘nonsense’ apples, just set the list to the empty list, since you then fire off two commands you’ll end up with two apples. Whenever an apple is eaten, you do not need to record this fact, just remove it and fire off a command to create a new one.
Secondly, you don’t really need to use commands for this. If you store the ‘seed’ on your model, you can just call Random.step
which will give you a new random value and the new seed to update on your model.
I’m sure there are other nit-picky things I could state such as:
getApplePositions : Model -> List Position
getApplePositions model =
List.map (\apple -> apple.position) model.apples
can be written as List.map .position model.apples
but if you’re happy with the above then that’s also fine. And most other stuff is mostly personal preference.