-
Notifications
You must be signed in to change notification settings - Fork 5
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
Update add command to MVP specifications #73
Update add command to MVP specifications #73
Conversation
Update multiple classes to ensure cohesion with Optional<Email> and Optional<Address>. Edit print statements so success output looks correct. Parameters not supplied will not be printed as output.
Deconflict merge issues by integrating optional email and address fields with new DateOfLastVisit field. Update SampleDataUtil and PersonUtil for sample people to have both optional emails and addresses, as well as DateOfLastVisit
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 really good job on the use of optionals for email and address. No major issues from what I can see, just some minor change(s) to be made. Good job!
@@ -41,16 +41,37 @@ public static String format(Person person) { | |||
final StringBuilder builder = new StringBuilder(); | |||
builder.append(person.getName()) |
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.
I like that you have abstracted out the builder.append(person) and builder.append('some text') for email, address, tags and date of last visit into a single method respectively. Perhaps you might want to do the same for name and phone too, just to make it consistent.
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.
LGTM
This PR changes the parameters address and email to be optional. That requires changing to many different classes and their interactions.
The outcome of this is that you can now create contacts with the most barebones information: name, phone number, and date of last visit. Email and address can still be supplied, but attempts to use the prefixes without supplying actual arguments results in error messages.
Closes #50
Closes #74