Feedback for Milestone 3

[john-breton]

Hello Group 23,

I have just finished reviewing your Milestone 3 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: 7/10

Overall Feedback

• Your JAR works and is feature complete. We went looking a bit harder for design and code smells this time around. I fear that you may have headed in the wrong direction with your attempts to improve the Property creation (detailed further below) but I still believe it is possible to get back on the right track. Please take my feedback into account for the next milestone.

Feedback Breakdown

Bugs Encountered

• Your quit button doesn't seem to actually do anything (I can quit on all players and the game continues) I'd recommend just getting rid of it.

Code Issues (-2)

• (-1) Major Design/Code Smells
  • I see what you are doing having the different Property tiles as their own classes, but now you have a lot of repeated code for the methods they implement. What could be done is to make Property an Abstract class that gets extended with the command functionality being present in the abstract class (like getCostPerHouse, toString, etc.) It would save on a lot of code duplication.
    • When I gave some feedback about how to improve the Property creation, I more so had in mind that the Property names (the Strings) could have been made into an ArrayList or HashMap (alongside the colours) that could be iterated over to build your Properties. Taking it out to their own classes is a bit extreme and results in an explosion of classes. You could have also had lists for the rent and cost that could also be iterated over in tandem. Just enough data structures defined as constants that you create methods in Board could make use of to avoid having such a large number of classes.
  • Your card frames also contain a lot of repeated code and occasionally empty methods. The alternative is to just not have those methods as part of your interface if you don't want every class that implements it to have them (you can just define them for that one class).
    • You also have a board field for each of these, but why? All you do is set this.board = board and then call a getProperty in the same method. You don't need to keep it as a field (better yet, just pass the Property as a parameter and call the constructor from a class that already has a board instance, which is what you do currently in MainFrame). It would reduce a lot of coupling that currently exists in your design.
    • Maybe consider just having a single Card class that is parameterized to support the functionality you need? I only suggest this because I noticed the repeated code.
• (-1) No tests were submitted with this milestone. There need to be tests for everything that falls under the Model present in your code (so your Board, Player, Jail, etc.)
• Missing features
  • AI isn't currently implemented which is mentioned in the README and I understand why it wasn't submitted, I will be giving some leniency here given the circumstances.
    • I'd caution about having the logic for your AI in your Game class. Your game is responsible for tracking player positions on the board, not for making the AI move. Consider looking into the Strategy pattern as it may prove helpful here.

Documentation

• (-1) Sequence Diagram
  • You need lifeline rectangles on your sequence diagram. Additionally, this was mentioned in my previous feedback but "sequence diagrams normally show instances of objects and not the class itself. So you should have something like "player: Player" rather than just "player"". Please implement this for the next milestone submission.
    • This is -1 mark because my feedback was ignored from the previous milestones.
• Good work on your other documentation, although your class diagram is currently difficult to look at with all of the overlapping arrows. The feedback I gave above should help reduce the number of relationships required and will hopefully result in a cleaner diagram.

[ToobaKS]

Done:

• Decreased duplicate property classes to just PropertyTiles and CornerTiles Class
• Changed the Tiles interface to be abstract, decreased duplicate methods.
• Decreased the number of duplicate methods in Card frame by removing them from the interface.
• Got rid of the board instance from all the view, and instead sent just property as a parameter.
• Changed all the card view classes, to fit into three: CardFrame(holds view for Property, Railroad and Utility Cards), CornerFrame (Holds Go, Go to Jail and Free Parking), JailFrame(holds the Jail view)