WIP: [#26] Allow snake to eat #27
Conversation
nickgnd
force-pushed
the
feature/26-allow-snake-to-eat
branch
from
456cd98
to
2a7ff11
Compare
There was a problem hiding this comment.
Rock solid work already @nickgnd 🙇 🙇 🙇
Added some small suggestion here and there. There are two things I'd like to discuss:
- using a guard 🤔
- explaining new code snippets before they appear in the text 🤔
Any thoughts on this ☝️
|
|
||
| state | ||
| |> put_in([:objects, :snake, :body], new_body) | ||
| |> maybe_eat_food(new_head) |
There was a problem hiding this comment.
A bit torn about the "maybe" part after reading https://ulisses.dev/elixir/2020/02/19/elixir-style-for-maintanability.html... 😉
There was a problem hiding this comment.
👍 Should we drop the maybe?
state
|> put_in([:objects, :snake, :body], new_body)
|> eat_pellet(new_head)What do. you think?
There was a problem hiding this comment.
For now not necessary in my opinion. I think the names are a bit off here - e.g. move_snake does more than moving the snake - but in general this is still fine I'd say.
The "problem" with maybe_ that the post tasks about also does not really fit our case I'd say.
There was a problem hiding this comment.
For now not necessary in my opinion. I think the names are a bit off here - e.g. move_snake does more than moving the snake - but in general this is still fine I'd say.
Good point, what about advance_snake ?
The "problem" with
maybe_that the post tasks about also does not really fit our case I'd say.
Sorry, but I didn't get your comment. Do you mean that the function name does not match the current use case since it does more stuff like placing the new pellet? 🤔
I see the point, but unfortunately I cannot came with a better name.
I kinda like that the grow_snake and place_pellet functions are grouped together since they are related. What about maybe_grow_snake or check_xxx dunno 😞
There was a problem hiding this comment.
I guess whether it's "move" or "advance" does not really matter. As said, both is fine here in my opinion.
My point with the maybe_ part was: the blog post has a more obvious scenario where one can get rid of the "maybe" part. Our case is different, we want always run this check.
Therefore: let's stick with the current naming - sorry for the confusion 🙈
Thanks @klappradla Co-Authored-By: Max Mulatz <klappradla@posteo.net>
|
@klappradla I updated the chapter based on your review, thanks a ton, really 💚 I still have one doubt. In the chapter sometimes we use Apart from that, I would prefer to don't merge this PR until the previous chapter has been drafted, I can take care of it in the next days. |
Morning 🌞 here the draft for the 6th chapter: allow snake to eat.
Adress #26