nus-cs2113-AY2223S2 / pe-dev-response

0 stars 0 forks source link

Implementation - integration with WellNUS++: error in class diagram #768

Open nus-pe-bot opened 1 year ago

nus-pe-bot commented 1 year ago

Note from the teaching team: This bug was reported during the Part II (Evaluating Documents) stage of the PE. You may reject this bug if it is not related to the quality of documentation.


I believe it is technically incorrect for an abstract class to be able to contain another object since it cannot be initialized? Might be better to have the arrows point from the subclasses to CommandParser instead.

image.png


[original: nus-cs2113-AY2223S2/pe-interim#1033] [original labels: severity.VeryLow type.DocumentationBug]

nichyjt commented 1 year ago

Team's Response

Thank you for your report!

Unfortunately, the team has decided to reject this bug for a few reasons.

Abstract Classes can hold other Objects

it is technically incorrect for an abstract class to be able to contain another object since it cannot be initialized

This is false. You are absolutely right to say that abstract classes cannot be initialized (with new). However, abstract classes can contain objects.

You can test this minimally by creating an abstract class with a String attribute (which is an object).

You can even instantiate the String attribute in the constructor (which MUST be non-abstract).

Supporting Documentation

We refer you to Java's official documentation on abstract classes.

In particular:

An abstract class is a class that is declared abstract—it may or may not include abstract methods.

This means that non-abstract methods can be defined in an abstract class that may or may not create valid objects such as a String or CommandParser within its logic.

Actual Implementation of Manager Class

Lastly,

Might be better to have the arrows point from the subclasses to CommandParser instead.

This cannot be done as it will be inaccurate in the context of our actual implementation for Manager and its subclasses. For Manager, you can see that there is an attribute protected CommandParser commandParser that is instantiated by its non-abstract constructor.

In all the implementations of Manager, when CommandParser is needed, it inherits the getCommandParser method which allows them access to the CommandParser built by Manager.

So, the class diagram was intentionally drawn as such to show that the CommandParser is inherited through Manager, to tell developers that there is no need to re-construct CommandParser in their XyzManager as its superclass has methods that allow them to access the underlying CommandParser.
Importantly, the implemented managers do not directly construct the command parser. That is only done by Manager, hence the only connecting line being from the Manager.

We hope this clarifies our reasons for rejecting this report. Thank you!

Duplicate status (if any):

--