-
Notifications
You must be signed in to change notification settings - Fork 10
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
[T4A1][T1-04] Aung Swumm #112
base: master
Are you sure you want to change the base?
Conversation
added interface "Printable" and overrides in detail classes
@@ -0,0 +1,14 @@ | |||
package seedu.addressbook.data.person; | |||
|
|||
//an interface that allows user to access data on a read-only, immutable basis |
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.
Good that you write the header comment, but try to follow the coding standard for this. Check out ReadOnlyPerson interface for reference.
for (Printable _printable : printables) { | ||
printableString.append(_printable.getPrintableString()); | ||
} | ||
return printableString.toString(); |
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.
This code shouldn't be here and also this method is never called. Discuss with your teammates.
@@ -42,6 +42,11 @@ public String toString() { | |||
} | |||
|
|||
@Override | |||
public String getPrintableString() { | |||
return "Address: " + this.toString(); | |||
} |
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.
Shouldn't you check whether it's private or not? When do you combine all the printables?
@ashpa Some comments added. Please close the PR after reading comments. Clean PR, good job. |
No description provided.