Skip to content
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

Use point struct as position #28

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

didiermichel
Copy link
Contributor

This PR intend is to improve code readability.
I introduce Point type that handle x and y value for an element (snake body, apple)

I hope it does bring more readability to the code.

@didiermichel
Copy link
Contributor Author

Note: code compiles, but I still need to flash and validate everything is still fine.

@deadprogram
Copy link
Member

Wouldn't it be better to use image.Point for this? https://pkg.go.dev/image#Point

@conejoninja
Copy link
Member

At the time I wrote the original code (many years ago 🤣 ), image.Point was causing some compilation errors, that should have been fixed by now, but, image.Point uses int instead of int16 (used by the display coords), casting will be needed in many places and in my opinion it will make readability worse. I vote for maintaining custom Point Struct instead of image.Point.

@didiermichel
Copy link
Contributor Author

I cannot say I had thought this through as I didn't know about image.Point 😆

@@ -29,6 +29,11 @@ const (
HEIGHTBLOCKS = 13
)

type Point struct {
Copy link
Member

@doniacld doniacld Oct 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For history of this structure, it could be nice to have a comment explaining why we are not using image.Point using the comment from @conejoninja.

Suggested change
type Point struct {
// Point is a custom type defining a point with x, y coordinates as int16 like the display coordinates.
type Point struct {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add the comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants