search

natlv / pe

0 stars 0 forks source link

'Person' could be replaced by 'Client' for greater clarity and standardisation #10

Open natlv opened 4 days ago

natlv commented 4 days ago

'Person' could be replaced by 'Client' for greater clarity and standardisation

Screenshot 2024-11-15 at 5.21.30 PM.png

Screenshot 2024-11-15 at 5.26.01 PM.png

Severity

Medium Reference – cs2103 bug severity levels

Desktop

nus-pe-script commented 19 hours ago

Team's Response

Thank you for your feedback. However, this issue does not constitute a bug. Below is the rationale for rejecting this report:

Reasons for rejecting

  1. The diagrams in the DG are visual representations of the actual code implementation. In our codebase, we intentionally retained the Person class rather than renaming it to Client. As such, the diagrams correctly reflect the naming convention used in the code.

Screenshot 2024-11-17 at 11.32.11 PM.png

  1. The use of Person instead of Client for our classes does not introduce any inaccuracy or error in the diagrams. It is a design decision that is consistent throughout the project and does not impact the clarity or functionality of the diagrams.

    Items for the Tester to Verify

    :question: Issue response

Team chose [response.Rejected]

Reason for disagreement: Thank you for your feedback. However, I believe this is still an issue with your documentation that should be accepted, albeit one that may stem from an underlying issue in code quality and copying of AB3.

I believe that renaming Person to Client and then refactoring the code could have been a much better approach, and would have made your developer guide much clearer.

Objectively and intuitively, it also does not make sense for the deleteClientCommand to lead to the execution of a command called deletePerson rather than deleteClient, given the way the Command is titled. Since I was not able to see your actual code in the PE, I could not verify whether the deletePerson command actually existed or you had forgotten to change its name. The latter is what a developer would likely think is the case when reading your developer guide, since it is obvious that a command titled "deleteClientCommand" should result in a method called "deleteClient" being called.

An alternative to countering this issue and reducing confusion could have been to simply specify that Person refers to Client in these diagrams, however this is not clearly specified.

## :question: Issue severity Team chose [`severity.VeryLow`] Originally [`severity.Medium`] - [x] I disagree **Reason for disagreement:** I do not believe that is a cosmetic issue (which severity.VeryLow is reserved for) because the inconsistency could confuse the developer, while deletePerson appears to reference a method that does not exist, since its execution arises from a Command called deleteClientCommand.