search

owx0130 / pe

0 stars 0 forks source link

incorrect format of attributes in Data component #25

Open owx0130 opened 5 months ago

owx0130 commented 5 months ago

image.png

it seems that some attributes (in SCREAMING_SNAKE case) are constants, so they should not be in the class diagram.

also, the Student class has a constant name which is Woi Ming. SCREAMING_SNAKE case is not used, and if it was supposed to be an object, the colon ";" was omitted in the label

nus-se-bot commented 5 months ago

Team's Response

Downgraded to low as it does not affect normal operations of the product. While it is true that the constant should not be present in the class diagram, it does not affect the overall diagram in a meaningful manner, and developers should still be able to understand the overall dependencies and associations in the diagram.

As for the ";" and SCREAMING_CASE, they are purely cosmetic issues and hence should be very low severity.

image.png

Items for the Tester to Verify

:question: Issue severity

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

Reason for disagreement: i disagree that omitting the colon : is purely cosmetic, as the user can interpret this diagram as a class or object diagram depending on whether the colon was omitted, which does cause a certain level of inconvenience and misinterpretation.

additionally, the Java Coding Standards (https://se-education.org/guides/conventions/java/basic.html) has outlined the need to reserve SCREAMING_SNAKE_CASE only for constants, which will confuse the user if used carelessly, thus I do not believe that using/not using SCREAMING_SNAKE_CASE is merely a cosmetic issue.

while including the constants does not affect the overall diagram, it does confuse the user's interpretation of the affected classes. For example, in DataHandler, the constant DATA_FILE_PATH was written in SCREAMING_SNAKE_CASE, which should give the user a first impression of it being a constant. However, there is no convention in CS2113 of including constants in the diagrams, which might leave the user second-guessing whether the developer team intended for this or that they forgot to use camelCase for it instead (to indicate an attribute/method).

all in all, i disagree that this issue should be downgraded to severity low, and believe that it causes enough inconvenience to the user that it classifies as a medium severity bug