Peer Review 2 workshop2 Patrik Nilsson - Toubic/Workshop-2-Design GitHub Wiki
workshop2 Patrik Nilsson
Try to compile/use the source code using the instructions provided. Can you get it up and running? Is anything problematic? Are there steps missing or assumptions made?
I can get it up and running and it works.
Does the implementation and diagrams conform (do they show the same thing)? Are there any missing relations? Relations in the wrong direction? Wrong relations? Correct UML notation?
The diagrams and implementation conform in general, there are some things left out but the focus was supposed to be on relations so that is not a problem. Considering the UML notation there are no multiplicity values on relations whatsoever. Which would have made the class diagram easier to understand [1, s.250].
In the interaction diagrams, precising a bit more what is being sent in some cases would probably be helpful and there is no use of dashed arrows for return values at all. Which would have made it more clear what is being returned in the interaction diagrams. "If you want to show that the receiver has finished processing the message and returns control to the sender, draw a dashed arrow from receiver to sender. Optionally, a value that the receiver returns to the sender can be placed near the return arrow." [2]. I think there is a missing return value from "Member" to "BoatClub" in "newMemberSequence.pdf".
Is the Architecture ok?
The architecture is a bit confusing with the naming a controller class "User". It could easily be mistaken for being a model class. Renaming it to include something like controller within the name and also include what it controls would be helpful. Applying packages for wrapping views, controllers and models could also help to enhance the understanding of the MVC architecture in this solution: [3]
Is there a model view separation?
There is a model view separation but as explained above it could be more clearer.
Is the model coupled to the user interface?
None of the models "Boat", "Member" and "BoatClub" are coupled to the user interface which is good according to the Model-View Separation Principle. "Do not connect or couple non-UI objects directly to UI objects." and "Why? Because the windows are related to a particular application, while (ideally) the non-windowing objects may be reused in new applications or attached to a new interface." [1, p.329].
Is the model specialized for a certain kind of IU (for example returning formated strings to be printed)
The model is designed with the console in mind and even have a class called "Console".
Are there domain rules in the UI?
No there is no business login in the UI which is good: "Do not put application logic (such as a tax calculation) in the UI object methods." [1, p.329].
Is the requirement of a unique member id correctly done?
Yes! Every member gets an unique id, auto incremented. The only way to break this is if you manipulate the JSON file.
What is the quality of the implementation/source code?
Naming could be improved otherwise I think it is good quality in the implementation. Like mentioned earlier naming a controller class "User" it could easily be mistaken for being a model class. Renaming it to include something like controller within the name and also include what it controls would be helpful. The "BoatClub" class which is strongly connected to the class "Member" could also be renamed to something like "MemberList" to really show the connection.
What is the quality of the design? Is it Object Oriented?
The classes have high cohesion which is a part of good quality design in object orientation [1, p.324] and objects are connected using associations not ids.
Is GRASP used correctly?
Classes have high cohesion and low coupling that is part of the nine GRASP patterns according to Rao [4, s.2]. Creator classes are used [4, s.4] and a controller class as well [4, s.16]. No use of static variables but there is a use of a static method called "isNumeric" in "User.java". That functionality could have been provided from a library instead. Private members are accessed via setters and getters but there are not much control on what is set in the setters.
As a developer would the diagrams help you and why/why not?
The classes in the diagram are detailed but the diagrams lacks a bit on explanations of relations. Like multiplicity values [1, s.250] in the class diagram and precising a bit more what is being sent in the interaction diagrams in some cases. Also there is no use of dashed arrows at all in the interaction diagrams which would have made it more clear what is being returned.
So declaring the classes will be easy but understanding the connections between classes might be harder.
What are the strong points of the design/implementation, what do you think is really good and why?
-
Using MVC pattern.
-
Detailed classes.
-
Low coupling between classes.
-
High cohesion in classes.
What are the weaknesses of the design/implementation, what do you think should be changed and why?
-
Confusing naming of classes like the controller class "User" and "BoatClub".
-
Details about relations and no dashed return arrows.
-
Static method called "isNumeric" in "User.java".
-
No packages around views, models and controllers in the class diagram, which would have made the diagram more organized.
-
Possibly a missing return value from "Member" to "BoatClub" in "newMemberSequence.pdf"
Do you think the design/implementation has passed the grade 2 criteria?
After fixing the weaknesses I think the solution has passed the grade 2 criteria.
References:
-
Larman C., Applying UML and Patterns: An Introduction to Object-Oriented Analysis and Design and Iterative Development, Third Edition, 2004-10-20, https://aanimesh.files.wordpress.com/2013/09/applying-uml-and-patterns-3rd.pdf
-
A Quick Introduction to UML Sequence Diagrams, 2015-02-16, http://www.tracemodeler.com/articles/a_quick_introduction_to_uml_sequence_diagrams/
-
Bulka A., puremvcfussUMLstep5takestock.png, 2012-07-29, http://www.atug.com/images/PureMvcRefactorImages/puremvcfussUMLstep5takestock.png
-
Rao D., GRASP Design Principles, 2012-12-09, https://www.cs.colorado.edu/~kena/classes/5448/f12/presentation-materials/rao.pdf