-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: [#26] Allow snake to eat #27
base: master
Are you sure you want to change the base?
Conversation
456cd98
to
2a7ff11
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
@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