[W5][T08-3]Julian Lim - #30
Conversation
| + "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"; |
There was a problem hiding this comment.
Small typo here.
| public static final String MESSAGE_PERSON_ALREADY_FAVOURITED = "Peron is already favourited"; | |
| public static final String MESSAGE_PERSON_ALREADY_FAVOURITED = "Person is already favourited"; |
| * Signals that the person is already favourited. | ||
| */ | ||
| public static class AlreadyFavouritedException extends DuplicateDataException { | ||
| protected AlreadyFavouritedException() { super("Peron is already favourited"); } |
There was a problem hiding this comment.
| protected AlreadyFavouritedException() { super("Peron is already favourited"); } | |
| protected AlreadyFavouritedException() { super("Person is already favourited"); } |
| 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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Neh. He is catching for the case of having an outdated list.
ie,
- You list 5 people
- You delete person 2
- 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)
0WN463
left a comment
0WN463
left a comment
There was a problem hiding this comment.
Overall, good attempt. I/O testing was in place and function-wise, the feature works, however you should be careful of the code smell that exists within your work
| * `find Betsy` + | ||
| `favourite 1` + | ||
| Favourites the 1st person in the results of the `find` command. | ||
|
|
There was a problem hiding this comment.
Substantial updating of UG while keeping to its original format, well done.
| 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) { |
There was a problem hiding this comment.
Neh. He is catching for the case of having an outdated list.
ie,
- You list 5 people
- You delete person 2
- 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); | ||
| } |
There was a problem hiding this comment.
Well done improving your user experience by catching for AlreadyFavourited cases
|
|
||
| public void favouritePerson(ReadOnlyPerson toFavourite) throws PersonNotFoundException, AlreadyFavouritedException { | ||
| allPersons.favourite(toFavourite); | ||
| } |
There was a problem hiding this comment.
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
| if(internalList.get(internalList.indexOf(toFavourite)).getFavourite()) { | ||
| throw new AlreadyFavouritedException(); | ||
| } else { | ||
| final boolean personFoundAndFavourited = internalList.get(internalList.indexOf(toFavourite)).setFavourite(); |
There was a problem hiding this comment.
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.
| this.isFavourite = true; | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
Odd choice of having a function that always returns true
| favourite 0 | ||
| favourite 3 | ||
| favourite 2 | ||
|
|
There was a problem hiding this comment.
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.)
|
See my comments for suggestions. Please close this PR after reading comments. |
5 participants
No description provided.