search

DrWala / pe

0 stars 0 forks source link

Storage component should be using Associations instead of Dependencies #16

Open DrWala opened 3 years ago

DrWala commented 3 years ago

What is the issue

Storage component depicts dependency between JsonSerializablePerson, JsonAdaptedTag, etc (Pg 9) image.png

What it should be

These links are likely to be associations instead of dependencies.

Why it matters

The developer reading this should not be misled into thinking the relationship that of a dependency.

Severity justification

Low because it will cause a minor inconvenience to developers trying to understand the project, not VeryLow as its not purely a cosmetic issue.

nus-pe-bot commented 3 years ago

Team's Response

This is dependent on the backend code.

Items for the Tester to Verify

:question: Issue response

Team chose [response.Rejected]

Reason for disagreement: While it was not possible to look at your code during PE, this issue was reported for AB3 at https://github.com/nus-cs2103-AY2021S2/forum/issues/305 (by me). Thus, I was suspicious of the diagram as it had not been updated with the correct associations.

Furthermore, as the team's response said, it is dependent on the backend code which is shown here. It confirms my suspicion that the diagram is invalid.

The link to the line showing that the association is incorrect https://github.com/AY2021S2-CS2103-W16-4/tp/blob/f8c015c90717f695cb9e0f00fdbc8696f26a5043/src/main/java/seedu/address/storage/JsonSerializableAddressBook.java#L26.

The image for your easy viewing: image.png