-
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][T01-T3] Teo Shu Qi #91
base: master
Are you sure you want to change the base?
Conversation
@Override | ||
public String getPrintableString() { | ||
return LABEL + 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 if it's a private or not? The variable name LABEL
is too generic.
@@ -83,5 +83,22 @@ public int hashCode() { | |||
public String toString() { | |||
return getAsTextShowAll(); | |||
} | |||
|
|||
private String addDetailToDetailsString(String details, String detail) { |
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.
No need to create a new method for this. Also the method name is quite confusing. :)
*/ | ||
private String getPrintableString(Printable ... printable) { | ||
String details = ""; | ||
for ( Printable detail :printable) { |
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.
Pay attention to the white space.
private String getPrintableString(Printable ... printable) { | ||
String details = ""; | ||
for ( Printable detail :printable) { | ||
addDetailToDetailsString(details, detail.getPrintableString()); |
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.
You might use java collection method, like String.join or StringBuilder.
} | ||
|
||
/** | ||
* Print out the details of the person requested. |
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 method doesn't print anything.
/** | ||
* Print out the details of the person requested. | ||
* @param printable | ||
* @return a concatenated version of the printable strings of each object. |
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.
You can remove @return
and write Returns ...
as the description.
public interface Printable { | ||
|
||
// Label stating what detail is this detail (e.g. Phone: ) | ||
public static String LABEL = ""; |
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.
No need to define it here.
@teoshuqi |
No description provided.