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

[W5][T08-3]Julian Lim #30

Open
wants to merge 4 commits 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
20 changes: 20 additions & 0 deletions docs/UserGuide.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,26 @@ Deletes the 2nd person in the address book.
`delete 1` +
Deletes the 1st person in the results of the `find` command.

== Favouriting a person : `favourite`

Favourites the specified person from the address book. Irreversible. +
Format: `favourite INDEX`

****
Favourites the person at the specified `INDEX`.
The index refers to the index number shown in the most recent listing.
****

Examples:

* `list` +
`favourite 2` +
Favourites the 2nd person in the address book.

* `find Betsy` +
`favourite 1` +
Favourites the 1st person in the results of the `find` command.

Copy link

Choose a reason for hiding this comment

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

Substantial updating of UG while keeping to its original format, well done.

== View non-private details of a person : `view`

Displays the non-private details of the specified person. +
Expand Down
43 changes: 43 additions & 0 deletions src/seedu/addressbook/commands/FavouriteCommand.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package seedu.addressbook.commands;

import seedu.addressbook.common.Messages;
import seedu.addressbook.data.person.ReadOnlyPerson;
import seedu.addressbook.data.person.UniquePersonList.PersonNotFoundException;
import seedu.addressbook.data.person.UniquePersonList.AlreadyFavouritedException;

/**
* Favourites a person identified using it's last displayed index from the address book.
*/
public class FavouriteCommand extends Command {

public static final String COMMAND_WORD = "favourite";

public static final String MESSAGE_USAGE = COMMAND_WORD
+ ": Favourites the person identified by the index number used in the last person listing.\n"
+ "Parameters: INDEX\n"
+ "Example: " + COMMAND_WORD + " 1";

public static final String MESSAGE_FAVOURITE_PERSON_SUCCESS = "Favourited %1$s";
public static final String MESSAGE_PERSON_ALREADY_FAVOURITED = "Peron is already favourited";

Choose a reason for hiding this comment

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

Small typo here.

Suggested change
public static final String MESSAGE_PERSON_ALREADY_FAVOURITED = "Peron is already favourited";
public static final String MESSAGE_PERSON_ALREADY_FAVOURITED = "Person is already favourited";


public FavouriteCommand(int targetVisibleIndex) {
super(targetVisibleIndex);
}


@Override
public CommandResult execute() {
try {
final ReadOnlyPerson target = getTargetPerson();
addressBook.favouritePerson(target);
return new CommandResult(String.format(MESSAGE_FAVOURITE_PERSON_SUCCESS, target));
} catch (IndexOutOfBoundsException ie) {
return new CommandResult(Messages.MESSAGE_INVALID_PERSON_DISPLAYED_INDEX);
} catch (PersonNotFoundException pnfe) {

Choose a reason for hiding this comment

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

The favourite command is based on the index shown from a list of people who exists in the address book so would there be a PersonNotFoundException? Wouldn't the index out of bounds exception be enough if they enter an invalid index?

Copy link

Choose a reason for hiding this comment

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

Neh. He is catching for the case of having an outdated list.
ie,

  1. You list 5 people
  2. You delete person 2
  3. You try to favourite person 2.

As the list is only updated per call to list, the list may be outdated and still contain non-existent people.
Good job catching for this edge case (How much is inherited, I will not comment)

return new CommandResult(Messages.MESSAGE_PERSON_NOT_IN_ADDRESSBOOK);
} catch (AlreadyFavouritedException pnfe) {
return new CommandResult(MESSAGE_PERSON_ALREADY_FAVOURITED);
}
Copy link

Choose a reason for hiding this comment

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

Well done improving your user experience by catching for AlreadyFavourited cases

}

}
6 changes: 5 additions & 1 deletion src/seedu/addressbook/data/AddressBook.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import seedu.addressbook.data.person.UniquePersonList;
import seedu.addressbook.data.person.UniquePersonList.DuplicatePersonException;
import seedu.addressbook.data.person.UniquePersonList.PersonNotFoundException;

import seedu.addressbook.data.person.UniquePersonList.AlreadyFavouritedException;
/**
* Represents the entire address book. Contains the data of the address book.
*/
Expand Down Expand Up @@ -54,6 +54,10 @@ public void removePerson(ReadOnlyPerson toRemove) throws PersonNotFoundException
allPersons.remove(toRemove);
}

public void favouritePerson(ReadOnlyPerson toFavourite) throws PersonNotFoundException, AlreadyFavouritedException {
allPersons.favourite(toFavourite);
}
Copy link

Choose a reason for hiding this comment

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

Some muddiness here because favourite can be both a verb, an adjective and a noun. So maybe renaming it to makeFavourite would make it clearer that it is a method and adhere better to coding standards


/**
* Clears all persons and tags from the address book.
*/
Expand Down
9 changes: 8 additions & 1 deletion src/seedu/addressbook/data/person/Person.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public class Person implements ReadOnlyPerson {
private Phone phone;
private Email email;
private Address address;

private boolean isFavourite = false;
private final Set<Tag> tags = new HashSet<>();

/**
Expand Down Expand Up @@ -62,6 +62,13 @@ public Set<Tag> getTags() {
return new HashSet<>(tags);
}

public boolean setFavourite() {
this.isFavourite = true;
return true;
}

Copy link

Choose a reason for hiding this comment

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

Odd choice of having a function that always returns true

public boolean getFavourite() { return this.isFavourite; }

/**
* Replaces this person's tags with the tags in the argument tag set.
*/
Expand Down
24 changes: 24 additions & 0 deletions src/seedu/addressbook/data/person/UniquePersonList.java
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,13 @@ protected DuplicatePersonException() {
}
}

/**
* Signals that the person is already favourited.
*/
public static class AlreadyFavouritedException extends DuplicateDataException {
protected AlreadyFavouritedException() { super("Peron is already favourited"); }

Choose a reason for hiding this comment

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

Suggested change
protected AlreadyFavouritedException() { super("Peron is already favourited"); }
protected AlreadyFavouritedException() { super("Person is already favourited"); }

}

/**
* Signals that an operation targeting a specified person in the list would fail because
* there is no such matching person in the list.
Expand Down Expand Up @@ -122,6 +129,23 @@ public void remove(ReadOnlyPerson toRemove) throws PersonNotFoundException {
}
}

/**
* Favourites the equivalent person from the list.
*
* @throws PersonNotFoundException if no such person could be found in the list.
* @throws AlreadyFavouritedException if person is already favourited.
*/
public void favourite(ReadOnlyPerson toFavourite) throws PersonNotFoundException, AlreadyFavouritedException {
if(internalList.get(internalList.indexOf(toFavourite)).getFavourite()) {
throw new AlreadyFavouritedException();
} else {
final boolean personFoundAndFavourited = internalList.get(internalList.indexOf(toFavourite)).setFavourite();
Copy link

Choose a reason for hiding this comment

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

The above violates Law of Demeter, which probably isn't taught yet.
That's why the better approach would be UniquePersonList telling target Person to make himself a favourite, and have the target Person be the one who throws the exception instead
Also, internalList.get(internalList.indexOf(toFavourite)) is called a lot so refactoring it out would be good.
Also the smell from the setFavourite also return true has resurfaced here. personFoundAndFavourited will always be true unless you run into a OOB Exception, which means that PersonNotFound Exception will never thrown and the program would malfunction.

if (!personFoundAndFavourited) {
throw new PersonNotFoundException();
}
}
}

/**
* Clears all persons in list.
*/
Expand Down
21 changes: 21 additions & 0 deletions src/seedu/addressbook/parser/Parser.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import seedu.addressbook.commands.DeleteCommand;
import seedu.addressbook.commands.ExitCommand;
import seedu.addressbook.commands.FindCommand;
import seedu.addressbook.commands.FavouriteCommand;
import seedu.addressbook.commands.HelpCommand;
import seedu.addressbook.commands.IncorrectCommand;
import seedu.addressbook.commands.ListCommand;
Expand Down Expand Up @@ -85,6 +86,9 @@ public Command parseCommand(String userInput) {
case FindCommand.COMMAND_WORD:
return prepareFind(arguments);

case FavouriteCommand.COMMAND_WORD:
return prepareFavourite(arguments);

case ListCommand.COMMAND_WORD:
return new ListCommand();

Expand Down Expand Up @@ -174,6 +178,23 @@ private Command prepareDelete(String args) {
}
}

/**
* Parses arguments in the context of the delete person command.
*
* @param args full command args string
* @return the prepared command
*/
private Command prepareFavourite(String args) {
try {
final int targetIndex = parseArgsAsDisplayedIndex(args);
return new FavouriteCommand(targetIndex);
} catch (ParseException pe) {
return new IncorrectCommand(String.format(MESSAGE_INVALID_COMMAND_FORMAT, FavouriteCommand.MESSAGE_USAGE));
} catch (NumberFormatException nfe) {
return new IncorrectCommand(MESSAGE_INVALID_PERSON_DISPLAYED_INDEX);
}
}

/**
* Parses arguments in the context of the view command.
*
Expand Down
24 changes: 24 additions & 0 deletions test/expected.txt
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@
|| Enter command: || [Command entered: delete 1]
|| The person index provided is invalid
|| ===================================================
|| Enter command: || [Command entered: favourite 1]
|| The person index provided is invalid
|| ===================================================
|| Enter command: || [Command entered: view 1]
|| The person index provided is invalid
|| ===================================================
Expand Down Expand Up @@ -233,6 +236,27 @@
||
|| 2 persons listed!
|| ===================================================
|| Enter command: || [Command entered: favourite]
|| Invalid command format!
|| favourite: Favourites the person identified by the index number used in the last person listing.
|| Parameters: INDEX
|| Example: favourite 1
|| ===================================================
|| Enter command: || [Command entered: favourite should be only one number]
|| The person index provided is invalid
|| ===================================================
|| Enter command: || [Command entered: favourite -1]
|| The person index provided is invalid
|| ===================================================
|| Enter command: || [Command entered: favourite 0]
|| The person index provided is invalid
|| ===================================================
|| Enter command: || [Command entered: favourite 3]
|| The person index provided is invalid
|| ===================================================
|| Enter command: || [Command entered: favourite 2]
|| Favourited Charlie Dickson Phone: (private) 333333 Email: [email protected] Address: 333, gamma street Tags: [school][friends]
|| ===================================================
|| Enter command: || [Command entered: delete]
|| Invalid command format!
|| delete: Deletes the person identified by the index number used in the last person listing.
Expand Down
17 changes: 17 additions & 0 deletions test/input.txt
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

# should show appropriate message when no listing has happened yet
delete 1
favourite 1
view 1
viewall 1

Expand Down Expand Up @@ -106,6 +107,22 @@
# find multiple with some keywords
find Charlie Betsy

##########################################################
# test favourite person command
##########################################################

# last active view: [1] betsy [2] charlie

# should catch invalid args format
favourite
favourite should be only one number

# should catch invalid index
favourite -1
favourite 0
favourite 3
favourite 2

Copy link

Choose a reason for hiding this comment

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

Good that I/O testing is updated
(Though because you didn't test for PersonNotFound, I can safely assumed that my thesis about "inheriting" is correct in my previous comment.)

##########################################################
# test delete person command
##########################################################
Expand Down