[W5][M11-3] Dannica Ong - #39
Conversation
|
Hi @dannong, your pull request title is invalid. For PR sent to satisfy a learning outcome, the PR name should be in the format of For team PR, the PR name should be in the format of Please follow the above format strictly and edit your title for reprocessing. Note: this comment is posted by a bot. If you believe this is done in error, please create an issue at nus-se-pr-bot and add a link to this PR. |
wanchinglim
left a comment
wanchinglim
left a comment
There was a problem hiding this comment.
Useful enhancement of adding the "Gender" option! Makes it easier if I have 2 friends of the same unisex name but of different genders. Code is easy to understand too.
|
hi @dannong interesting add of person’s gender which can be beneficial for one to differentiate when adding contacts! |
There was a problem hiding this comment.
Good PR which adds a gender field to the Addressbook Person format.
Try to have more fine-grained and detailed commits. At present there is just one commit which inhibits possibilities to track incremental updates. Also, please note the proper format to write commit messages . In particular, write the summary line and description of what you have done in the imperative mode, that is as if you were commanding someone. Start the line with "Fix", "Add", "Change" instead of "Fixed", "Added", "Changed".
Minor comments are added for you to look at. Please close this PR after reading them.
| import seedu.addressbook.data.person.Phone; | ||
| import seedu.addressbook.data.person.ReadOnlyPerson; | ||
| import seedu.addressbook.data.person.UniquePersonList; | ||
| import seedu.addressbook.data.person.*; |
There was a problem hiding this comment.
Importing everything using "*" (also known as wildcard imports) is generally discouraged and it also violates coding style. It reduces interpretability as to what packages are imported. Please note that your IDE might tend to create this error. Find out how to avoid it (https://stackoverflow.com/questions/3348816/intellij-never-use-wildcard-imports).
| } | ||
|
|
||
| @Override | ||
| public Gender getGender() { return gender; } |
There was a problem hiding this comment.
Probably would be better to stick to the K&R style of brackets as seen in the adjacent methods.
| assertConstructingInvalidAddCmdThrowsException(Name.EXAMPLE, gender, Phone.EXAMPLE, true, Email.EXAMPLE, true, | ||
| Address.EXAMPLE, false, EMPTY_STRING_SET); | ||
| } | ||
| } |
There was a problem hiding this comment.
Good job on working with JUnit tests!
| /** | ||
| * Represents a Person's gender in the address book. | ||
| * Guarantees: immutable; is valid as declared in {@link #isValidGender(String)} | ||
| */ |
There was a problem hiding this comment.
Good job for including class comments.
5 participants
No description provided.