[W6.2i][T09-4] Vincent Neo #125
Conversation
…at returns String. Also added a function in Person to combine the printables into 1 string and called the function during view command
|
|
||
| public static final String EXAMPLE = "123, some street"; | ||
| public static final String MESSAGE_ADDRESS_CONSTRAINTS = "Person addresses can be in any format"; | ||
| public static final String ADDRESS_VALIDATION_REGEX = ".+"; | ||
| public static final String ADDRESS_PRINTABLE_LABEL = "ADDRESS"; |
There was a problem hiding this comment.
Good job on extracting out the label as a constant
| * A read-only immutable interface for any field that can be printed in the addressbook. | ||
| * Implementations should guarantee: details are present and not null, field values are validated. | ||
| */ | ||
| public interface Printable { |
There was a problem hiding this comment.
Good job on adding header comments for both Class and method
| @@ -61,6 +61,15 @@ public Address getAddress() { | |||
| return new HashSet<>(tags); | |||
| } | |||
|
|
|||
| public String getPrintableString(Printable ...printables){ | |||
| String printableString = printables[0].getPrintableString(); | |||
There was a problem hiding this comment.
This is correct. But to be consistent with all the instances in the list, you may create a default string and update while iterating the entire list. This will help you in future in case if you want to do something else within the loop for all the elements in the array
| @@ -21,6 +21,8 @@ | |||
| */ | |||
| Set<Tag> getTags(); | |||
|
|
|||
| String getAsTextNoRestrictions(); | |||
There was a problem hiding this comment.
You may have the implementation here rather than in Person since printing does not mutate the data (Any behaviour that does not mutate the Person can be kept in ReadOnlyPerson)
| @@ -55,4 +56,8 @@ public int hashCode() { | |||
| public boolean isPrivate() { | |||
| return isPrivate; | |||
| } | |||
|
|
|||
| public String getPrintableString() { | |||
There was a problem hiding this comment.
Better to have the @OverRide keyword to explicitly specify to the reader that this is an overridden method. Moreover, if you make a common mistake of misspelling a method name or not correctly matching the parameters, you will be warned.
| @@ -32,7 +32,7 @@ public CommandResult execute() { | |||
| if (!addressBook.containsPerson(target)) { | |||
| return new CommandResult(Messages.MESSAGE_PERSON_NOT_IN_ADDRESSBOOK); | |||
| } | |||
| return new CommandResult(String.format(MESSAGE_VIEW_PERSON_DETAILS, target.getAsTextHidePrivate())); | |||
| return new CommandResult(String.format(MESSAGE_VIEW_PERSON_DETAILS, target.getAsTextNoRestrictions())); | |||
There was a problem hiding this comment.
Good that you attempted to use the getPrintableString with this new method. However, this changes the behavior of View command. Please note that going forward you need to update the tests as well when you make any behavior changes.
[Addressbook-Level3: LO-Interfaces]