-
Notifications
You must be signed in to change notification settings - Fork 73
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
[W7.8][W12-3]Ler Wei Sheng #145
base: master
Are you sure you want to change the base?
Conversation
Ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the javafx code is fine.
However, your PR is not clean as it contains unrelated code.
Try to have a clean PR next time.
You can close this PR.
@Override | ||
public void changed(ObservableValue<? extends String> observable, | ||
String oldValue, String newValue) { | ||
// System.out.println(newValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove this instead of just comment out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there are some code related to the interface task, I have commented on the Printable
interface code as well.
@@ -6,6 +6,8 @@ | |||
import seedu.addressbook.data.person.UniquePersonList.DuplicatePersonException; | |||
import seedu.addressbook.data.person.UniquePersonList.PersonNotFoundException; | |||
|
|||
import java.util.Set; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused import
@@ -55,4 +55,11 @@ public int hashCode() { | |||
public boolean isPrivate() { | |||
return isPrivate; | |||
} | |||
|
|||
@Override | |||
public String getPrintableString (Printable... printables) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of taking in printables
argument?
@@ -58,4 +58,11 @@ public int hashCode() { | |||
public boolean isPrivate() { | |||
return isPrivate; | |||
} | |||
|
|||
@Override | |||
public String getPrintableString (Printable... printables) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: printables
is unused
@@ -61,4 +61,9 @@ public int hashCode() { | |||
return fullName.hashCode(); | |||
} | |||
|
|||
@Override | |||
public String getPrintableString (Printable... printables) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
printables
is unused
|
||
|
||
@Override | ||
public String getPrintableString (Printable... printables) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
printables
is unused
No description provided.