nus-pe-bot
commented
3 days ago Team's Response
Thanks for raising this issue! Firstly, I would disagree that the loop label is misleading and it should not need to explain why it is necessary for, because the loop label is just meant to show the condition for the loop to be running.
Secondly, I think the main point of this sequence diagram was to show how the addCommand sequence goes and it is to show how things would go if the appropriate values were inputted into the system. Hence, there isn't a need to show the opt frame, as the point was not to show how exceptions work. and the exceptions will be covered more in depthly in the Exception component.
Items for the Tester to Verify
:question: Issue response
Team chose [response.Rejected]
- [x] I disagree
Reason for disagreement: While I agree with the severity being downgraded to VeryLow, I feel like adding at least a UML note explaining why the checking is being done would help make a lot more sense to someone who is reading the DG and has no access to the codebase itself. A simple mention about the checking for duplicate student ID's (even without the mention of invalid inputs/exceptions) could've made the second part of the diagram much clearer. This is because, without this crucial information, it is unclear as to why the existing students in the StudentList are even being checked. At first glance, it seems like you are simply checking if the students present in the StudentList are valid which is something that will always be true since they are already in the StudentList. Thus, this seemed like a redundant loop to me until I went to the codebase on GitHub and figured out the real reason. Simply avoiding the part about this loop would've also been an easy fix and wouldn't have caused any confusion since it was stated by the dev team in the response that the main point of the diagram was to show how the addCommand sequence goes and not the working of the StudentList class. So, they could have simply avoided explaining how the StudentList function addStudent(newStudent) works in their sequence diagram. The team giving incomplete partial information on what this method actually does is what I feel causes the confusion and the resulting bug
## :question: Issue severity Team chose [`severity.VeryLow`] Originally [`severity.Low`] - [ ] I disagree **Reason for disagreement:** [replace this with your explanation]
The loop label seems to be misleading and does not describe why it is actually necessary to check all students. Adding an opt block inside the loop to mention the checking of the equality of student id's and including a return to the user inside the block because of a throw statement could help make the UML diagram clearer