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

[T4A1][T1-04] Aung Swumm #112

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

ashpa
Copy link

@ashpa ashpa commented Jul 3, 2017

No description provided.

ashpa added 2 commits July 2, 2017 12:01
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

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();

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();
}
Copy link

@jeffryhartanto jeffryhartanto Jul 8, 2017

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?

@jeffryhartanto
Copy link

@ashpa Some comments added. Please close the PR after reading comments. Clean PR, good job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants