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

Refactor tile class #53

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ksk5280
Copy link
Collaborator

@ksk5280 ksk5280 commented May 31, 2016

What's this PR do?

Removed extra arguments out of the tile constructor now it just takes in a tile type. Moved the tile types into another file. It's not being used now, but will be used when we add validations for placing tiles only next to matching sides. Changed the tests to reflect the changes.

Where should the reviewer start?

Start by looking at the tile class, then the tile inventory class.

How should this be manually tested?

Play the game and make sure the tiles still show up and are playable.

Any background context you want to provide?

Another refactor that had to be added because tiles were not able to be played on occasion: On line 65 and 66 of the tile class, I divided and multiplied by 10 to make the numbers round to the nearest 10. When they were rounding to the nearest 1, sometimes they wouldn't be placed close enough together and this caused an alert saying that the tile placement was invalid.

gif

@rrgayhart
@adriennedomingus
@scottfirestone

closes #51

ksk5280 added 3 commits May 31, 2016 10:40
Removed excess args from tile constructor.
Edited get position function in tile to round to nearest 10.
U: { quantity: 8 },
V: { quantity: 9 },
W: { quantity: 4 },
X: { quantity: 1 }

Choose a reason for hiding this comment

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

This is a lot cleaner. Awesome

@rrgayhart
Copy link

Good refactor here - would be good to address the concerns pointed out by Adrienne and then 🚢

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.

Refactor tile class constructor
3 participants