search

LWS49 / pe

0 stars 0 forks source link

UML diagram for weight does not reflect functionality #13

Open LWS49 opened 5 months ago

LWS49 commented 5 months ago

image.png

If the index is invalid, the Invalid index error should always be raised first based on this diagram.

However, that is not the case here:

image.png

nus-se-script commented 5 months ago

Team's Response

image.png

This bug only occurs in rare situations when both the index AND the weight are invalid, and only when index is wrong by being higher than the list size. If index is 0 or less, the diagram will be correct.

Considering that the bug only occurs when:

We find that it is more reasonable to say that it occurs in very rare situations and not occasionally. Furthermore, this only causes a minor inconvenience.

Thus, we have accepted the bug, but downgraded the severity from Medium --> Low.

Items for the Tester to Verify

:question: Issue severity

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

Reason for disagreement: The issue at hand is not whether the test cases happens rarely, but whether the diagram is accurate. I find this diagram to be misleading, and may cause a misunderstanding for developers who notice the incongruency between the UML diagram and the actual code.

Looking at your code, lines 47-49 of the screenshot below from WeightCommand is what raised the error:

image.png

An invalid index can mean either an unparseable index (of which all cases get picked up by parseIndex throwing IllegalValueException error), or an index that does not make sense in the context of the commmand (which include both unparseable index and index that is parseable but larger than the size of the list).

Looking at the full UML diagram below, this UML only has one instance of mentioning invalid index. Hence, it may be natural for developers to intrepret what you meant by invalid index as the latter (in the previous paragraph), when you meant the former. Otherwise, nowhere else in your diagram is the application supposed to reject the index that is parseable but larger than the list size. This is what I thought as well.

image.png

I only understood why this is the case by looking at the code, and realizing that lines 47-49 are missing from the diagram. Although it is specified for us to omit less important details, I do think that WeightCommand checking index and throwing INVALID_INDEX error is important. Without which, and due to the ambiguity of the meaning "invalid index", this diagram becomes inaccurate.

Thus, I believe this is a medium severity, as even though the use case that shows how this diagram is rare, the problem is that this diagram is inaccurate, and causes developers to miss potentially crucial information on how the weight tracking feature works. Hence, I will say that this is a medium bug that "causes occasional inconvenience to some readers", since developers reading this UML diagram will have a inaccurate understanding of weight tracking feature.