m1oojv / pe

0 stars 0 forks source link

DG : missing destroy of XYZ Command after result is given #22

Open m1oojv opened 8 months ago

m1oojv commented 8 months ago

Screenshot 2023-11-17 at 5.14.39 PM.png

nus-se-script commented 7 months ago

Team's Response

The XYZCommand does not get deleted when the result is returned. As seen in the docs, the X when used in Java's context can be used (i.e. it is optional) to show that an object is no longer being used (hence that is why it was used earlier for XYZCommandParser). Screenshot 2023-11-18 at 11.33.12 PM.png Thus, as it is optional, it was omitted in this case, especially since there is no explicit deletion as seen in the code.

Screenshot 2023-11-18 at 11.35.50 PM.png

Items for the Tester to Verify

:question: Issue response

Team chose [response.Rejected]

Reason for disagreement: Dear Team,

While I appreciate your explanation regarding the optional nature of the destroy marker ('X') in UML diagrams, especially in the context of a language like Java with automatic memory management, I believe there is still a significant reason to consider the inclusion of the destroy marker for the 'XYZ Command' in your diagram. The key concern here is the consistency and clarity of the UML representation for readers, particularly those who may not be familiar with the intricacies of Java's memory management.

Consistency in Diagrams: The inclusion of a destroy marker for the 'XYZ Command Parser' but not for the 'XYZ Command' could lead to confusion. Readers might infer that the two objects have different lifecycles or roles within the system. Consistent use of destroy markers, where applicable, aids in providing a clear and unified understanding of the object lifecycles in the system.

Clarity of Object Lifecycle: Even in languages with automatic memory management, the use of a destroy marker can be helpful to indicate when an object ceases to be relevant within the context of the diagram. This is particularly important for understanding the flow of the application and the lifespan of different objects within it.

Educational Value: For educational purposes, especially for those learning UML or system design, the explicit representation of object destruction—even if it's managed automatically by the language—can be invaluable. It provides a complete picture of object management within the application, enhancing the educational utility of the diagram.

Best Practices in UML Diagramming: Adhering to best practices in UML diagramming entails providing as much relevant information as possible to accurately represent the system's behavior. Omitting the destroy marker where it could be applicable might oversimplify the representation or mislead the reader regarding the object's lifecycle.

Given these points, while the omission of the destroy marker for 'XYZ Command' might technically be acceptable, its inclusion would enhance the clarity, consistency, and educational value of the UML diagram. I recommend revisiting this aspect of the diagram to ensure it accurately reflects the lifecycle of all objects involved, aligning with best practices in UML representation.

Additionally, I'd like to highlight the guidance provided by the professor regarding the handling of bugs. A rejection of a bug should be reserved for cases where the bug is deemed irrelevant or unnecessary to address, both in the current context and in the foreseeable future. However, in this instance, the issue at hand is directly relevant and necessary to address.

Screenshot 2023-11-22 at 11.59.26 AM.png

The omission of the destroy marker for 'XYZ Command' in the UML diagram, while perhaps technically optional, has significant implications for the clarity and educational value of the diagram.

Screenshot 2023-11-22 at 12.00.33 PM.png

In fact, in the course website , it says to be consistent with any decision made by the team on the standards and conventions, in order not to confuse the user , which clearly the dev team has failed to comply with.


## :question: Issue severity Team chose [`severity.Low`] Originally [`severity.Medium`] - [ ] I disagree **Reason for disagreement:** [replace this with your explanation]