Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[W6.2b][F09-3] Sarah Taaher Bonna #134

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SarahTaaherBonna
Copy link

No description provided.

Copy link

@fanyiii fanyiii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revisit the part on polymorphism and consider how this method can be better implemented using overriding.

}
// public CommandResult execute(){
// throw new UnsupportedOperationException("This method should be implement in child classes");
// }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't leave commented out code. Just go ahead and delete unused code, we can recover any deleted code later.

@@ -70,4 +73,10 @@ public int getTargetIndex() {
public void setTargetIndex(int targetIndex) {
this.targetIndex = targetIndex;
}

public boolean isMutating(String userCommandText) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method should have a header comment. If not, users of the method as well as those who overrides method will not know how exactly the method behaves.


public boolean isMutating(String userCommandText) {
this.userCommandText = userCommandText;
if(userCommandText == "add" || userCommandText == "delete") return true;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to use overriding in subclasses to specify the different behavior of this method instead (it will not need to take in any argument), so that when you add any new command, you won't have to change parts outside of that class

@@ -82,10 +82,10 @@ public CommandResult execute(String userCommandText) throws Exception {
* @return result of the command
* @throws Exception if there was any problem during command execution.
*/
private CommandResult execute(Command command) throws Exception {
private CommandResult execute(Command command, String userCommandText) throws Exception {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to update the header comments accordingly

} catch (UniquePersonList.DuplicatePersonException dpe) {
return new CommandResult(MESSAGE_DUPLICATE_PERSON);
}
}

@Override
public boolean isMutating(String userCommandText) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is actually no polymorphism as the subclass simply calls the super class method. A better way to specify the behaviors of different commands would be to remove the default implementation in super class, and having each sub class returning True/False.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants