Feedback for Milestone 1

[john-breton]

Hello Group 23,

I have just finished reviewing your Milestone 1 submission. Included below is my feedback for your submission. Please ensure all issues I have brought up are addressed in your next milestone submission!

If you have any questions or concerns, feel free to ping me on Discord or email me at johnbreton@cmail.carleton.ca

Mark: 10/10

Overall Feedback

• Great start so far! Just a few issues that will need to be addressed for future milestones but your design is taking shape!

Feedback Breakdown

Bugs Encountered

• Just a minor error-checking bug at the start when specifying the number of players but nothing game-breaking!
• I'll post the crash as a separate issue so you can easily track and fix it before the next milestone submission.

Code Issues

• It would be nice to know which properties a player owns when you get their info. That might be helpful to include for future milestones.
• Your createProperties method is very smelly! Hard-coding these values and then adding them one by one is a nightmare for maintainability. For future milestones, you should look into ways to extract this setup out of this method (maybe using enums or even reading from a file might be helpful here, but those are just some suggestions).
• You might run into some issues without having a more general way of identifying spaces on a board. Currently, everything is a property so that works fine, but there could be issues when you have special spaces that need to be added for later milestones.
• It doesn't appear like anything happens if a player rolls doubles. In the rules, rolling doubles allows players to take another turn after they have finished their first turn. You'll need to add this in future milestones.
• Game could use a bit of refactoring. For example, given your other fields, currentPlayer isn't necessary as that information can be determined from currentPlayerIndex and the players' list.

UML

• Your sequence diagram is not correct. Lifelines need to have rectangles to signify the extent of their interactions with other objects (check for examples online that explain this in more detail).
  • One minor nitpick is that sequence diagrams normally show instances of objects and not the class itself. So you should have something like "player: Player" rather than just "player"
  • Additionally, after "checks if command is valid" returns to board, command somehow messages property with "Buy/Sell". This shouldn't be possible unless Command received another message to supersede this message pass.
• The composition relationships between Property and Player should both be aggregated.
  • To illustrate this, imagine if a Player is eliminated from the game. The properties they owned are not destroyed when they are kicked out if the game is still going. They still exist on the board for other Players, and they can even get bought by other Players. This is why it is an aggregation rather than a composition. It would be a composition if the Properties that a Player owned were destroyed when the Player is destroyed, but that isn't what happens in Monopoly (or at least not any Monopoly games I've played before).
• You didn't include your main method in your UML for the Game class. Although not strictly required, including that does help to identify the entry point into your application. This should be included in future milestones going forward.

[ToobaKS]

Code Stuff Done

• Number of players bug fixed
• Printing properties owned by player done
• Double dice roll implemented
• Refactored to remove extra fields

Stuff Left

• Smelly code left
• General board tiles will add in next milestone