search

AY1920S2-CS2103T-W12-1 / main

Delino - Managing Delivery Tasks
https://ay1920s2-cs2103t-w12-1.github.io/main/
MIT License
0 stars 6 forks source link

DG Review #308

Open nishantbudhdev opened 4 years ago

nishantbudhdev commented 4 years ago

Here are some comments and questions that may help you improve your DG.

  1. Is your architecture the same as AddressBook? If not, please update the architecture diagram.

  2. Suggestion: ReturnOrder and Order have a lot of common attributes. Can you think of a way to make a better OOP solution?

    Screenshot 2020-04-06 at 1 08 34 PM

  3. Suggestion: Remove unconnected components. Or break this diagram to multiple UML diagrams and explain each of them individually.

    Screenshot 2020-04-06 at 1 11 26 PM

  4. Sec 2.6 -> Classes used by multiple components are in the seedu.addressbook.commons package. Do you still use the addressbook as the package name? Otherwise good job in updating the content to remove references to AddressBook application.

  5. Too many details in the diagram. Remove getter/setter functions, trivial data members. Also, split the diagram into multiple UML diagrams since there are two disjoint sets of classes. Use similar methods for simplifying other class diagrams.

    Screenshot 2020-04-06 at 1 14 12 PM

  6. Suggestion: Reduce verbosity and details. For example, "Creates new InsertCommand object and calls execute method." (Figure 10) Use similar techniques for other boxes in the activity diagram throughout the DG.

    Screenshot 2020-04-06 at 1 18 51 PM

  7. Suggestion: Remove implicit arguments. Therefore you can omit arguments for both parseCommand() and parse(). (Figure 11)

    Screenshot 2020-04-06 at 1 22 08 PM

  8. Are you missing an conditional box for true and false here? (Figure 17)

    Screenshot 2020-04-06 at 1 24 18 PM

  9. Is this the correct composition direction arrow?

    Screenshot 2020-04-06 at 1 33 38 PM

  10. What is sd Execution of the Import Command Sequence Diagram?

    Screenshot 2020-04-06 at 1 29 14 PM

  11. Great job on Appendix G.

Overall try and reduce the complexity and details in your UML diagrams. Everything else looks good. 👍

Exeexe93 commented 4 years ago

Regarding the number 10 points, the sd is for the reference at the import command sequence diagram at the top of the diagram.

image

Furthermore, how could I show the exception path if it is being thrown in the sequence uml diagram?

khsc96 commented 4 years ago

@nishantbudhdev Hi nishan regarding point 9, is a static class embedded in a class not a composition? Because EditParcelDescriptor is a sub static class in EditCommand class.