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.1d][T15-B2] Cheng Jin Ting #917

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/seedu/addressbook/commands/AddCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -66,5 +66,8 @@ public CommandResult execute() {
return new CommandResult(MESSAGE_DUPLICATE_PERSON);
}
}

@Override
public boolean isMutating () {
return true ;
}
}
5 changes: 5 additions & 0 deletions src/seedu/addressbook/commands/ClearCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,9 @@ public CommandResult execute() {
addressBook.clear();
return new CommandResult(MESSAGE_SUCCESS);
}

@Override
public boolean isMutating () {
return true ;
}
}
4 changes: 4 additions & 0 deletions src/seedu/addressbook/commands/Command.java
Original file line number Diff line number Diff line change
Expand Up @@ -70,4 +70,8 @@ public int getTargetIndex() {
public void setTargetIndex(int targetIndex) {
this.targetIndex = targetIndex;
}


//Making the method abstract forces all child classes to declare whether it is a mutating command or not, rather than depend on an arbitrary decision made in the parent class.
public abstract boolean isMutating () ;
}
Copy link

Choose a reason for hiding this comment

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

A more precise phrasing is Returns true if … (instead of Checks if …)
It is better to mention it returns true if the Command potentially mutates the data (header comments specify a contract, so try to be as precise as possible). E.g. Adding a duplicate person will not mutate data but the corresponding AddCommand object will still return true for this method.
@return … line can be omitted for this method as it says the same thing as the first sentence.

5 changes: 5 additions & 0 deletions src/seedu/addressbook/commands/DeleteCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,9 @@ public CommandResult execute() {
}
}

@Override
public boolean isMutating () {
return true ;
}

}
4 changes: 4 additions & 0 deletions src/seedu/addressbook/commands/ExitCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,8 @@ public CommandResult execute() {
return new CommandResult(MESSAGE_EXIT_ACKNOWEDGEMENT);
}

@Override
public boolean isMutating () {
return false ;
}
}
5 changes: 5 additions & 0 deletions src/seedu/addressbook/commands/FindCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,9 @@ private List<ReadOnlyPerson> getPersonsWithNameContainingAnyKeyword(Set<String>
return matchedPersons;
}

@Override
public boolean isMutating () {
return false ;
}

}
5 changes: 5 additions & 0 deletions src/seedu/addressbook/commands/HelpCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,9 @@ public class HelpCommand extends Command {
public CommandResult execute() {
return new CommandResult(MESSAGE_ALL_USAGES);
}

@Override
public boolean isMutating () {
return false ;
}
}
4 changes: 4 additions & 0 deletions src/seedu/addressbook/commands/IncorrectCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,8 @@ public CommandResult execute() {
return new CommandResult(feedbackToUser);
}

@Override
public boolean isMutating () {
return false;
}
}
5 changes: 5 additions & 0 deletions src/seedu/addressbook/commands/ListCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,9 @@ public CommandResult execute() {
List<ReadOnlyPerson> allPersons = addressBook.getAllPersons().immutableListView();
return new CommandResult(getMessageForPersonListShownSummary(allPersons), allPersons);
}

@Override
public boolean isMutating () {
return false;
}
}
6 changes: 6 additions & 0 deletions src/seedu/addressbook/commands/ViewAllCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,10 @@ public CommandResult execute() {
return new CommandResult(Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX);
}
}


@Override
public boolean isMutating () {
return false;
}
}
5 changes: 4 additions & 1 deletion src/seedu/addressbook/commands/ViewCommand.java
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,8 @@ public CommandResult execute() {
return new CommandResult(Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX);
}
}

@Override
public boolean isMutating () {
return false ;
}
}
4 changes: 3 additions & 1 deletion src/seedu/addressbook/logic/Logic.java
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,9 @@ public CommandResult execute(String userCommandText) throws Exception {
private CommandResult execute(Command command) throws Exception {
command.setData(addressBook, lastShownList);
CommandResult result = command.execute();
storage.save(addressBook);
if ( command.isMutating()) {
Copy link

Choose a reason for hiding this comment

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

Coding standard violations! Please refer the coding standard.

storage.save(addressBook);
}
return result;
}

Expand Down
7 changes: 6 additions & 1 deletion test/java/seedu/addressbook/logic/LogicTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import seedu.addressbook.data.tag.Tag;
import seedu.addressbook.data.tag.UniqueTagList;
import seedu.addressbook.storage.StorageFile;
import seedu.addressbook.parser.Parser;

import java.util.*;

Expand Down Expand Up @@ -90,7 +91,11 @@ private void assertCommandBehavior(String inputCommand,
//Confirm the state of data is as expected
assertEquals(expectedAddressBook, addressBook);
assertEquals(lastShownList, logic.getLastShownList());
assertEquals(addressBook, saveFile.load());
//to create a new Command instance
if (new Parser().parseCommand(inputCommand).isMutating()) {
assertEquals(addressBook, saveFile.load());
}

Copy link

Choose a reason for hiding this comment

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

This change doesn’t help much because it does not confirm that the saving happens only for mutating commands. Tests will pass even if you don’t have this change. To be complete, you should have an else branch that confirms the file was not updated for non mutating commands (e.g. by checking the last updated time of the file).

}


Expand Down