search

chiageng / pe

0 stars 0 forks source link

UML diagram for Storage missing out Crucial information #18

Open chiageng opened 2 months ago

chiageng commented 2 months ago

Justification : Developer will always refer to DG in order to get overview of storage model

image.png

image.png

I believe you missed out JsonAdaptedTransaction in this diagram, as this is one of your most important item in your architecture

(Umm, honestly it is hard to spot this mistake since you all are doing a really good job...)

nus-se-script commented 2 months ago

Team's Response

Thanks for the feedback. However, we contend that the absence of JsonAdaptedTransaction in the diagram has minimal impact on the reader. The developer guide serves as a supplementary resource to our codebase and developers are expected to delve into the codebase for detailed understanding. JsonAdaptedTransaction functions similarly to JsonAdaptedPerson, and its exclusion from the diagram does not impede comprehension, given the assumed familiarity of developers with the codebase.

Items for the Tester to Verify

:question: Issue severity

Team chose [severity.Low] Originally [severity.Medium]

Reason for disagreement: Developer will depend on this diagram to understand the architecture of current app.

assumed familiarity of developers with the codebase should not be an assumption. A new developer joining the team will fail to follow the architecture design and causing your app to be buggy if new developer push his code without fully understand the structure of app.

Transaction and Person are totally separated Class in your implementation. There are multiple designs which are possibly implemented in this case.

  1. Storing transaction as a field in Person, which mean you will dont have any JsonAdaptedTransaction at all
  2. Storing transaction as independent JsonAdaptedTransaction, link Transaction with Person using Association class (Your current implementation)

Developer will not be able to know which implementation to follow if the UML diagram by omiting such important details, if developer mistaken as 1st implementation, then the code implementation in future will be expected to be very buggy. This will cause a big misunderstanding for developer. Hence the severity will not be low as this directly impact developer's works.

There is no clear explanation in DG on how you implement JsonAdaptedTransaction which will cause confusion. DG is meant to help developer to understand the main architecture, and should not be confusing.