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 enums for representing the state of a figure #44

Merged
merged 5 commits into from
Jul 27, 2023

Conversation

guemax
Copy link
Collaborator

@guemax guemax commented Jul 26, 2023

Enums provide the benefit of not being prone to accidental changes by a programmer who sets the boolean flags previously used by Figure in a wrong manner, similar to this:

figure[i].inHouse = true;
// And somewhere down the same program flow
figure[i].inBase = true;

This can be a real problem, because a figure can never be in its house and in its base at the same time!

Since enums either take one value or another, this problem can easily be prevented. It also serves to produce much cleaner code, which I have amplified by moving this logic to the class Figure, which should be responsible for managing its own state.

@guemax guemax force-pushed the use-enums-for-state-of-figure branch 3 times, most recently from 7659d1d to b91993a Compare July 26, 2023 20:53
@guemax guemax force-pushed the use-enums-for-state-of-figure branch from b91993a to abe84f0 Compare July 27, 2023 04:50
Copy link
Owner

@MeiNic MeiNic left a comment

Choose a reason for hiding this comment

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

Great job.👍
The changes make a lot of sense, so the described scenario can certainly not occur, where that was currently prevented by the code. I did not know the data type "enum" until this code review😂. I would still wait for another review

@MeiNic MeiNic requested a review from TastingComb July 27, 2023 05:55
@TastingComb TastingComb merged commit d122edc into MeiNic:master Jul 27, 2023
@guemax guemax deleted the use-enums-for-state-of-figure branch July 31, 2023 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants