search

jayasting98 / pe

0 stars 0 forks source link

`CommandResult` constructor in sequence diagrams return something and are not named #10

Open jayasting98 opened 2 years ago

jayasting98 commented 2 years ago

I don't think constructors are supposed to return anything. I think if you want to return them from execute, you should put the name of the instance in front of the colon in front of the class name. This happens in other sequence diagrams as well.

image.png

nus-pe-bot commented 2 years ago

Team's Response

The constructor does not need to have a name for the object created regardless if the object is returned or not as seen here.

Items for the Tester to Verify

:question: Issue response

Team chose [response.Rejected]

Reason for disagreement: I disagree because the response does not address my main issue; it seems like a strawman argument.

My main issue with the sequence diagrams is the fact that I do not think constructors return things. I think only methods return things. This means that this should not be valid:

image.png

My discussion about naming the object was merely to offer a possible fix to the potential documentation bug that may pop up when the returned commandResult is removed. If it is removed, then this reference to commandResult may no longer make sense or it may no longer be clear that it refers to that object just created:

image.png

Anyway, I am not sure whether it is even clear from the original diagram that this specific CommandResult object that was just created was the one returned as commandResult or if it is actually another variable called commandResult. Perhaps even if things could be returned from constructors, then the CommandResult should still be named to make that clear. Alternatively, they could opt to remove all references to commandResult completely. Regardless, I still do not think constructors return anything anyway but if that is not a bug then the fact that the diagram is not clear (whether due to the lack of a name or due to the use of potentially unknown references) is a bug.

Furthermore, the example the response cited does not even seem to look at a case where the Minefield object is returned from newGame(). In fact, I think it is more likely it is not returned and is actually just maintained as a private attribute of the Logic object.

image.png

On the other hand, these ones from some of the videos do, and it shows the objects are named if it is known that they are referenced (whether in the diagram or in the code):

image.png i is referenced in the code.

image.png t is referenced in the diagram as it is returned by generate.

In fact, I think that while in general, object naming is not necessary, in order to make it clear that the object returned is exactly the one created via the constructor, the created object should usually be named. So perhaps while this naming thing was not my main issue, maybe it could be one of my main issues with this as well.