search

JulietTeoh / pe

0 stars 0 forks source link

Low level details given in Undo Feature section #17

Open JulietTeoh opened 3 years ago

JulietTeoh commented 3 years ago

Low-level details like using a new Arraylist<>() should not be given in the DG, and using terms like UnmodifiableObservable List would have sufficed.

Screenshot 2021-04-16 at 3.36.25 PM.png

nus-pe-bot commented 3 years ago

Team's Response

Actual executing commands are listed here for a better explanation of the workflow. And previousAppointmentLists.push(new ArrayList<>(appointments.asUnmodifiableObservableList())) is the actual executing command to copy the appointments list and added to the previousAppointmentLists, thus, it cannot be changed to UnmodifiableObservable, which seems to be high level but actually incorrect. The explanation is crystal clear with the command previousAppointmentLists.push(new ArrayList<>(appointments.asUnmodifiableObservableList())).

Items for the Tester to Verify

:question: Issue response

Team chose [response.Rejected]

Reason for disagreement: https://github.com/nus-cs2103-AY2021S2/forum/issues/334

The documentation should be sufficiently high-level as the reader can refer to the code for lower-level details. That section of the DG is trying to explain to the reader how the undo command works, however, mentioning low-level data structure like ArrayList is not essential to that explanation and it would be better to use pseudocode. It is good for the reader to be able to understand how undo command has been implemented, but the explanation should be done in a high level manner instead of a low level one, where you mention pushing and into an arraylist.