Add outline for app-specific commands

[sandydays]

Part of #65

• Includes rough outline of parsing of add, delete, edit, find, and list for patients and appointments.
• Added view command
• Now all these commands operate on the list of persons in the address book. Once appointment and patient specific classes added to model, these commands will only modify those lists.
• Yet to change tests to fit the changed command parsing methods.
• This skeleton can be adopted for other similar commands like view-visit, etc. and modified later.
• Would like feedback on the OOP design too - considering packages for all add commands, all delete commands, and so on...or should it be packages for different models, like appointments, visits, patients, etc.?

[crazoter]

TL;DR
I'm sorry, but this PR is a mess (haha sorry if this comes across as harsh and/or savage but seriously though)

Details
There's a lot of duplicated problems, so I'll just lay it out here so you can fix it (see recommendations at the end of this review on what possible steps you can take in the future):

1. When you create a Command class, make sure that that class has the word "Command" at the end. For example, the AddAppointment class should be AddAppointmentCommand class. This issue has appeared multiple times.

2. Also, when creating your Command classes, put them into subpackage (not at the same level as logic.command) so it's neater. This is a minor organizational issue, however, compared to the rest.

3. AddCommand, EditCommand, DeleteCommand, FindCommand etc are not generic commands. You can think of it as the AB3 developer's lazy way of omitting the word Person. They are pseudonyms for AddPersonCommand, EditPersonCommand etc. Please read the code before blindly extending them for your use, because I see you have extended these classes for your Appointment Commands, which is just silly because these classes pertain to modifying Persons, which is different from Appointments. If you have the intent of making these classes generic then I apologise for the miscomm, but from what I see this was not the intent.

4. Also I see you've modified the MESSAGE_USAGE for these existing Commands e.g. AddCommand etc to include your message usage. So now it will print something like edit-pat|appt. Apart from the obvious notion that this is incorrect (it is a whole command as is, if you wanted to edit the MESSAGE_USAGE it should've been edit-pat or edit-appt and not a lazy merger of the two). The command should only deal with itself. In addition, the syntax as agreed upon is ENTITY-COMMAND, which is completely different from the changes proposed in this PR. In addition this was based on the assumption that these existing classes were generic (refer to point 3 above), which is invalid (i.e. don't edit the AddCommand etc Commands thinking they are generic Commands. They are not. They are AddPersonCommand etc for Persons.)

5. The assumptions made at points 3 and 4 culminate in the unnecessary / invalid changes made to the CommandParsers which leads to a lot of work done that is wasted.

I stopped adding code specific comments halfway through because the problems just kept repeating / kept escalating due to these invalid assumptions, so I've decided to put them all into this general review here for my (and your) convenience. Please revert and fix and please clarify before making any changes you are unsure of, because this PR is effectively useless considering almost all the observable changes are not in line with what we discussed and/or would not work, apart from the creation of AppointmentCommand class stubs (which again weren't fleshed out because their body was just copied).

What was good
Now, I would like to say I actually like the idea that you've tried to implement the COMMAND-ENTITY idea. It does feel usable. But it's not really in line with what we've discussed, and it does make it harder for the autocomplete, amongst other issues that made us choose to do ENTITY-COMMAND.

I would also like to praise you for the amount of work put in. It's no small amount to be certain.

But a mess is a mess and to avoid such scenarios from happening again, please ask/confirm in the group before making significant changes that can impact other parts of the code.

Recommendations on what to do next
I would recommend just reverting back to a previous commit-

No, I would actually recommend just ditching this branch and creating a new branch to start afresh from the main branch (after copying usable code from this PR to another folder somewhere for future reference) because all this extra work to fix what was done in this PR may not be worth it (apart from the stubs created, I don't see any content that can be kept). May want to keep the change for Ui.png though if you do this.

Thanks Sandy.

[crazoter]

AddAppointment should be named as "AddAppointmentCommand".

[crazoter]

The body of the function is understandably incomplete since it's a copy from the AddCommand, however, but don't forget the JavaDocs etc. You can replace them by selecting person using Alt+J and modifying as needed (this is case-sensitive, so make sure to select "person" and "Person")

[crazoter]

Also, why is AddAppointment extending AddCommand? Note that in the application, the "AddCommand" is effectively an "AddPersonCommand". Please do not be confused and think it is a generic class for adding items. It is not.

[crazoter]

I do not understand this. Commands should be separate. Why is the message usage here "add-pat|appt"? Didn't we agree on "pat-add" and "appt-add"?

[crazoter]

Note that in the application, the "AddCommand" is effectively an "AddPersonCommand". Please do not be confused and think it is a generic class for adding items. It is not.

[crazoter]

The consensus was Object, dash, then Command. Please correct me if I misinterpreted this.

[crazoter]

Don't forget to update the messages and JavaDocs.

[crazoter]

Object then Command

[crazoter]

object - word

[crazoter]

EditAppointmentCommand, and don't extend the EditCommand.

[crazoter]

object - word

[crazoter]

Don't forget to update the body and the javadocs

[crazoter]

I believe you can just remove this class and refactor the current AddCommand class.

[sandydays]

Hi Matt, thanks for the review and I am sorry if I had to make you read through many lines of code you found quite redundant and messy. I think in some way I was trying to just create some dummy classes that could be modified later and fleshed out properly later, but as I see that perhaps is not a very useful way to go about this, so next time I do put a PR, I will ensure as much as I can from my side that the code is mostly in shape. This was a WIP that was not ready at all to be merged, but to just get some feedback on. Clearly, there were a lot of misunderstandings on my side that surfaced through this PR, so I guess that perhaps was the only good thing out of this.

I would just like to clarify a few things as well based on your comments, if I am wrong on any of them please let me know:

• So it is object-word, like pat-add or appt-add
• When adding command classes, will ensure to add Command to the back of the class names. So I will name the classes as PatientAddCommand, AppointmentDeleteCommand, etc.
• You suggested packaging the different commands further. So now, since we are doing object-word, should they be packaged by object (i.e. appointment, patient, etc.) or by the action (add, edit, etc.)?
• Also, from the AddressBookParser class, do I split different kinds of commands by the action (add, edit, delete) or by object (patient, appointment, etc.)?
• I was in fact planning to use AddCommand class as a generic class to be extended by AddPatient and AddAppointment etc. just that I had not removed all the code within it and cleaned it up. But what is your suggestion on generic classes? Should we have a generic class like PatientCommand which all patient related commands extend from? Similarly for appointments etc.?
• Consequently, the changes with MESSAGE_USAGE was just a temporary thing I had done since I hadn't yet cleaned up AddCommand, DeleteCommand, etc. code yet.
• Will modify javadocs for any code changes made next time

Again, I am sorry. I believe this could have been quite frustrating and time consuming for you. This was more of an experiment on my side just to get some initial feedback. I will try to give better quality and cleaner code next time. And will try to push in small increments to avoid this. Will start fresh on a new branch as suggested and only focus on appointment commands first perhaps.

[crazoter]

Hi Sandy, don't worry. Regarding your questions:

• So it is object-word, like pat-add or appt-add

Yes, pat-add and appt-add

• When adding command classes, will ensure to add Command to the back of the class names. So I will name the classes as PatientAddCommand, AppointmentDeleteCommand, etc.

Yes thank you

• You suggested packaging the different commands further. So now, since we are doing object-word, should they be packaged by object (i.e. appointment, patient, etc.) or by the action (add, edit, etc.)?

When the I use the term "Package", I literally mean Java Packages (see https://www.w3schools.com/java/java_packages.asp) e.g.package seedu.address.logic.commands.
The term "subpackage" is a misuse on my part because each package are just packages with different names (see https://stackoverflow.com/questions/13809713/java-package-in-package), but I was referring to the notion that you have something like package seedu.address.logic.commands.appointment like what i've done with visits:

• Also, from the AddressBookParser class, do I split different kinds of commands by the action (add, edit, delete) or by object (patient, appointment, etc.)?

Neither, the implementation concept should be kept as is. Treat the command as a whole, because this extraction of entity and action is an additional feature that provides neither the user nor the developer any value at this point. appt-add should be recognized as appt-add, appt-delete should be recognized as appt-delete etc, not appt and then you search for add. What's the point of adding a dash if you're going to implement segregation of commands?

• I was in fact planning to use AddCommand class as a generic class to be extended by AddPatient and AddAppointment etc. just that I had not removed all the code within it and cleaned it up. But what is your suggestion on generic classes? Should we have a generic class like PatientCommand which all patient related commands extend from? Similarly for appointments etc.?

I believe it's not worth making a generic command for add etc in the first place. What will you extract out? Each entity has different variables, different descriptions, different constraints etc; if you look deeper it's really hard to see similarities on what you can "add". Maybe in v1.4 this can be considered, but at this point I recommend against generic classes.

• Consequently, the changes with MESSAGE_USAGE was just a temporary thing I had done since I hadn't yet cleaned up AddCommand, DeleteCommand, etc. code yet.
• Will modify javadocs for any code changes made next time

No hurry just something to take note of because it's easy to miss and forget

[crazoter]

As discussed during the afternoon meeting today, Sandy intends to make her changes on another branch. Closing this PR unless new info crops up