Friend/Unfriend Command + MePanel Features #68
Conversation
sync with upstream
Sync with upstream
cjinting-nus
added
type.story
| Person editedUser = UserStub.getUser(); | ||
| editedPerson.getFriends().add(new Friend(UserStub.getUser().getName())); | ||
| Person editedUser = model.getUser(); | ||
| addFriendToPerson(editedPerson, model.getUser()); |
There was a problem hiding this comment.
Wait, could you run me through what this part of the code does?
addFriendToPerson seems to be an empty method too...
There was a problem hiding this comment.
The command would add the current User to the target Person's friend list and viceversa
Empty method was not cleaned up, was meant to make the code cleaner
| private final FriendListPredicate friendListPredicate; | ||
|
|
||
| public CombinedFriendPredicate(Predicate<Person> predicate, FriendListPredicate friendListPredicate) { | ||
| this.predicate1 = predicate; |
There was a problem hiding this comment.
The 1 seems unnecessary here since the differentiation can be seen by explicitly writing this.predicate vs predicate
There was a problem hiding this comment.
That's true, thanks Nian Fei
| if (predicate1.test(person) && friendListPredicate.test(person)) { | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Probably cleaner to have a else here
Or even return predicate1.test(person) && friendListPredicate.test(person)
| for (Friend friend : friendList) { | ||
| if (currentUser.getName().equals(person.getName())) { |
There was a problem hiding this comment.
There's only 3 valid cases, right?
- Is himself
- Is friend
- Is not friend
I think it'd be easier to read if you made each of the 3 cases a branch in an if-else statement instead of returning false by default
| Person editedUser = UserStub.getUser(); | ||
| editedPerson.getFriends().add(new Friend(UserStub.getUser().getName())); | ||
| Person editedUser = model.getUser(); | ||
| addFriendToPerson(editedPerson, model.getUser()); |
| requireNonNull(predicate); | ||
| otherList.setPredicate(combinedOtherPredicate(predicate, othersPredicateFromPerson(user))); | ||
| } | ||
|
|
||
| @Override | ||
| public ObservableList<Person> getFriendList(Person person) { |
There was a problem hiding this comment.
I think I mentioned this #56 as well but in this context it seems weird to have to pass a person to the API method, given that all it should do update the app with the current User's friend list instead of any other Person's. Allowing methods to pass any Person seems to be unsafe if some other method accidently calls it with anyone other than the current User.
More things to carry to v1.4 I guess.
Softfix can be found in
MainWindowlines 201 & 205