zognin / pe

0 stars 0 forks source link

[DG] Filter class diagram does not seem to match implementation #11

Open zognin opened 2 years ago

zognin commented 2 years ago

The filter class diagram suggests that a MajorContainsKeywordPredicate contains 1 or more Majors.

Screenshot 2021-11-12 at 5.26.56 PM.png

However it seems like the list implementation allows the list to be empty or even null. So, there could be 0 majors.

Screenshot 2021-11-12 at 5.28.16 PM.png

The same occurs for all other predicates. For example this,

Screenshot 2021-11-12 at 5.29.20 PM.png

nus-pe-script commented 2 years ago

Team's Response

Thank you for reporting this issue.

However, if you try the command filter m/ with no keywords you would get an error because the parser does not accept the command with no keywords. This check happens in the parser class and MajorContainsKeywordPredicate would only be created if there were at least one or more majors present in the user input.

That is why the diagram states that MajorContainsKeywordPredicate can contain one or more majors. Because otherwise the class would not have been created to begin with.

The same thing applies to the other predicates.

Items for the Tester to Verify

:question: Issue response

Team chose [response.Rejected]

Reason for disagreement: The team's justified that MajorContainsKeywordPredicate must contain one or more majors based on whether the parser would create an instance of the predicate. This is a behaviour.

However, UML class diagrams should reflect the structure and not the behaviour of the OOP solution.

Screenshot 2021-11-17 at 12.53.48 PM.png

The implementation of the Predicate classes should be treated independently from the behaviour of the Parser class. The diagram incorrectly describes the structure of the Predicate classes to always have at least one predicate, when there is nothing in the class that enforces that.

Hence I disagree with the team's response.