Merge pull request #141 from chengda300/fix-addp-blanks

Fix add* to ignore optional fields if they are included but actually blank
AY2223S1-CS2103T-F11-1 · Oct 21, 2022 · d95eaf7 · d95eaf7
2 parents a2231b0 + 2cbe6fe
commit d95eaf7
Show file tree

Hide file tree

Showing 16 changed files with 91 additions and 86 deletions.
diff --git a/src/main/java/seedu/address/logic/parser/AddInternshipCommandParser.java b/src/main/java/seedu/address/logic/parser/AddInternshipCommandParser.java
@@ -51,11 +51,7 @@ public AddInternshipCommand parse(String args) throws ParseException {
                 ParserUtil.parseInternshipStatus(argMultimap.getValue(PREFIX_INTERNSHIP_STATUS).get());
 
         String interviewDateStr = argMultimap.getValue(PREFIX_INTERVIEW_DATE).orElse(null);
-        InterviewDate interviewDate = null;
-        if (interviewDateStr != null) {
-            interviewDate = ParserUtil.parseInterviewDate(interviewDateStr);
-        }
-
+        InterviewDate interviewDate = ParserUtil.parseInterviewDate(interviewDateStr);
         String linkIndexStr = argMultimap.getValue(PREFIX_LINK_INDEX).orElse(null);
         Index linkIndex = null;
         if (linkIndexStr != null) {

diff --git a/src/main/java/seedu/address/logic/parser/AddPersonCommandParser.java b/src/main/java/seedu/address/logic/parser/AddPersonCommandParser.java
@@ -46,15 +46,9 @@ public AddPersonCommand parse(String args) throws ParseException {
 
         Name name = ParserUtil.parseName(argMultimap.getValue(PREFIX_NAME).get());
         String emailStr = argMultimap.getValue(PREFIX_EMAIL).orElse(null);
-        Email email = null;
-        if (emailStr != null) {
-            email = ParserUtil.parseEmail(emailStr);
-        }
+        Email email = ParserUtil.parseEmail(emailStr);
         String phoneStr = argMultimap.getValue(PREFIX_PHONE).orElse(null);
-        Phone phone = null;
-        if (phoneStr != null) {
-            phone = ParserUtil.parsePhone(phoneStr);
-        }
+        Phone phone = ParserUtil.parsePhone(phoneStr);
         Set<Tag> tagList = ParserUtil.parseTags(argMultimap.getAllValues(PREFIX_TAG));
         String linkIndexStr = argMultimap.getValue(PREFIX_LINK_INDEX).orElse(null);
         Index linkIndex = null;

diff --git a/src/main/java/seedu/address/logic/parser/ParserUtil.java b/src/main/java/seedu/address/logic/parser/ParserUtil.java
@@ -98,14 +98,14 @@ public static Name parseName(String name) throws ParseException {
      * @throws ParseException if the given {@code phone} is invalid.
      */
     public static Phone parsePhone(String phone) throws ParseException {
-        if (phone != null) {
-            String trimmedPhone = phone.trim();
-            if (!Phone.isValidPhone(trimmedPhone)) {
-                throw new ParseException(Phone.MESSAGE_CONSTRAINTS);
-            }
-            return new Phone(trimmedPhone);
+        if (phone == null || phone.isBlank()) {
+            return new Phone(null);
+        }
+        String trimmedPhone = phone.trim();
+        if (!Phone.isValidPhone(trimmedPhone)) {
+            throw new ParseException(Phone.MESSAGE_CONSTRAINTS);
         }
-        return new Phone(null);
+        return new Phone(trimmedPhone);
     }
 
 
@@ -116,7 +116,9 @@ public static Phone parsePhone(String phone) throws ParseException {
      * @throws ParseException if the given {@code email} is invalid.
      */
     public static Email parseEmail(String email) throws ParseException {
-        requireNonNull(email);
+        if (email == null || email.isBlank()) {
+            return new Email(null);
+        }
         String trimmedEmail = email.trim();
         if (!Email.isValidEmail(trimmedEmail)) {
             throw new ParseException(Email.MESSAGE_CONSTRAINTS);
@@ -131,7 +133,9 @@ public static Email parseEmail(String email) throws ParseException {
      * @throws ParseException if the given {@code tag} is invalid.
      */
     public static Tag parseTag(String tag) throws ParseException {
-        requireNonNull(tag);
+        if (tag == null || tag.isBlank()) {
+            return null;
+        }
         String trimmedTag = tag.trim();
         if (!Tag.isValidTagName(trimmedTag)) {
             throw new ParseException(Tag.MESSAGE_CONSTRAINTS);
@@ -143,10 +147,11 @@ public static Tag parseTag(String tag) throws ParseException {
      * Parses {@code Collection<String> tags} into a {@code Set<Tag>}.
      */
     public static Set<Tag> parseTags(Collection<String> tags) throws ParseException {
-        requireNonNull(tags);
         final Set<Tag> tagSet = new HashSet<>();
         for (String tagName : tags) {
-            tagSet.add(parseTag(tagName));
+            if (tagName != null && !tagName.isBlank()) {
+                tagSet.add(parseTag(tagName));
+            }
         }
         return tagSet;
     }
@@ -159,7 +164,7 @@ public static Set<Tag> parseTags(Collection<String> tags) throws ParseException
      * @throws ParseException if the given {@code internshipId} is invalid.
      */
     public static InternshipId parseInternshipId(String internshipId) throws ParseException {
-        if (internshipId == null) {
+        if (internshipId.isBlank()) {
             return null;
         }
         String trimmedInternshipId = internshipId.trim();
@@ -221,7 +226,9 @@ public static InternshipStatus parseInternshipStatus(String internshipStatus) th
      * @throws ParseException if the given {@code interviewDate} is invalid.
      */
     public static InterviewDate parseInterviewDate(String interviewDate) throws ParseException {
-        requireNonNull(interviewDate);
+        if (interviewDate == null || interviewDate.isBlank()) {
+            return new InterviewDate(null);
+        }
         String trimmedInterviewDate = interviewDate.trim();
         if (!InterviewDate.isValidDatetimeStr(trimmedInterviewDate)) {
             throw new ParseException(InterviewDate.MESSAGE_CONSTRAINTS);

diff --git a/src/main/java/seedu/address/model/internship/Internship.java b/src/main/java/seedu/address/model/internship/Internship.java
@@ -66,6 +66,9 @@ public PersonId getContactPersonId() {
     }
 
     public InterviewDate getInterviewDate() {
+        if (interviewDate == null) {
+            return new InterviewDate(null);
+        }
         return interviewDate;
     }
 

diff --git a/src/main/java/seedu/address/model/internship/InterviewDate.java b/src/main/java/seedu/address/model/internship/InterviewDate.java
@@ -1,10 +1,10 @@
 package seedu.address.model.internship;
 
-import static java.util.Objects.requireNonNull;
 import static seedu.address.commons.util.AppUtil.checkArgument;
 
 import java.time.LocalDateTime;
 import java.time.format.DateTimeFormatter;
+import java.util.Objects;
 
 /**
  * Represents an Internship's interview date in the address book.
@@ -26,11 +26,14 @@ public class InterviewDate {
      * @param datetimeStr A valid String in the format yyyy-MM-dd HH:mm that represents a date and time.
      */
     public InterviewDate(String datetimeStr) {
-        requireNonNull(datetimeStr);
-        checkArgument(isValidDatetimeStr(datetimeStr), MESSAGE_CONSTRAINTS);
-
-        LocalDateTime datetime = LocalDateTime.parse(datetimeStr, formatter);
-        this.datetime = datetime;
+        if (datetimeStr == null || datetimeStr.isBlank()) {
+            this.datetime = null;
+        } else {
+            checkArgument(isValidDatetimeStr(datetimeStr), MESSAGE_CONSTRAINTS);
+
+            LocalDateTime datetime = LocalDateTime.parse(datetimeStr, formatter);
+            this.datetime = datetime;
+        }
     }
 
     public static boolean isValidDatetimeStr(String test) {
@@ -39,14 +42,15 @@ public static boolean isValidDatetimeStr(String test) {
 
     @Override
     public String toString() {
+        if (datetime == null) {
+            return "No interviews scheduled";
+        }
         return datetime.format(formatter);
     }
 
     @Override
     public boolean equals(Object other) {
-        return other == this // short circuit if same object
-                || (other instanceof InterviewDate // instanceof handles nulls
-                && datetime.equals(((InterviewDate) other).datetime)); // state check
+        return Objects.equals(datetime, ((InterviewDate) other).datetime);
     }
 
     @Override

diff --git a/src/main/java/seedu/address/model/person/Email.java b/src/main/java/seedu/address/model/person/Email.java
@@ -2,6 +2,8 @@
 
 import static seedu.address.commons.util.AppUtil.checkArgument;
 
+import java.util.Objects;
+
 /**
  * Represents a Person's email in the address book.
  * Guarantees: immutable; is valid as declared in {@link #isValidEmail(String)}
@@ -55,14 +57,15 @@ public static boolean isValidEmail(String test) {
 
     @Override
     public String toString() {
+        if (value == null) {
+            return "No email";
+        }
         return value;
     }
 
     @Override
     public boolean equals(Object other) {
-        return other == this // short circuit if same object
-                || (other instanceof Email // instanceof handles nulls
-                && value.equals(((Email) other).value)); // state check
+        return Objects.equals(value, ((Email) other).value);
     }
 
     @Override

diff --git a/src/main/java/seedu/address/model/person/Person.java b/src/main/java/seedu/address/model/person/Person.java
@@ -54,10 +54,16 @@ public Name getName() {
     }
 
     public Phone getPhone() {
+        if (phone == null) {
+            return new Phone(null);
+        }
         return phone;
     }
 
     public Email getEmail() {
+        if (email == null) {
+            return new Email(null);
+        }
         return email;
     }
 
@@ -119,15 +125,9 @@ public int hashCode() {
     @Override
     public String toString() {
         final StringBuilder builder = new StringBuilder();
-        builder.append(getName());
-        Email email = getEmail();
-        if (email != null) {
-            builder.append(("; Email: "));
-        }
-        Phone phone = getPhone();
-        if (phone != null) {
-            builder.append(("; Phone: "));
-        }
+        builder.append(getName())
+                .append(("; Email: ")).append(getEmail())
+                .append(("; Phone: ")).append(getPhone());
         Set<Tag> tags = getTags();
         if (!tags.isEmpty()) {
             builder.append("; Tags: ");

diff --git a/src/main/java/seedu/address/model/person/Phone.java b/src/main/java/seedu/address/model/person/Phone.java
@@ -2,6 +2,8 @@
 
 import static seedu.address.commons.util.AppUtil.checkArgument;
 
+import java.util.Objects;
+
 /**
  * Represents a Person's phone number in the address book.
  * Guarantees: immutable; is valid as declared in {@link #isValidPhone(String)}
@@ -20,7 +22,6 @@ public class Phone {
      * @param phone A valid phone number.
      */
     public Phone(String phone) {
-        //requireNonNull(phone);
         if (phone != null) {
             checkArgument(isValidPhone(phone), MESSAGE_CONSTRAINTS);
         }
@@ -36,14 +37,15 @@ public static boolean isValidPhone(String test) {
 
     @Override
     public String toString() {
+        if (value == null) {
+            return "No phone number";
+        }
         return value;
     }
 
     @Override
     public boolean equals(Object other) {
-        return other == this // short circuit if same object
-                || (other instanceof Phone // instanceof handles nulls
-                && value.equals(((Phone) other).value)); // state check
+        return Objects.equals(value, ((Phone) other).value);
     }
 
     @Override

diff --git a/src/main/java/seedu/address/ui/InternshipCard.java b/src/main/java/seedu/address/ui/InternshipCard.java
@@ -67,11 +67,7 @@ public void setContactPerson(String contactPersonName) {
         } else {
             contactPerson.setText("Contact Person: " + contactPersonName);
         }
-        if (internship.getInterviewDate() == null) {
-            interviewDate.setText("No interviews scheduled.");
-        } else {
-            interviewDate.setText("Interview on: " + internship.getInterviewDate().toString());
-        }
+        interviewDate.setText("Interview on: " + internship.getInterviewDate().toString());
     }
 
     @Override

diff --git a/src/main/java/seedu/address/ui/PersonCard.java b/src/main/java/seedu/address/ui/PersonCard.java
@@ -17,8 +17,6 @@ public class PersonCard extends UiPart<Region> {
 
     private static final String FXML = "PersonListCard.fxml";
     private static final String NO_INTERNSHIP = "No internship linked.";
-    private static final String NO_PHONE = null;
-    private static final String NO_EMAIL = null;
 
     /**
      * Note: Certain keywords such as "location" and "resources" are reserved keywords in JavaFX.
@@ -53,16 +51,8 @@ public PersonCard(Person person, int displayedIndex) {
         this.person = person;
         id.setText(displayedIndex + ". ");
         name.setText(person.getName().fullName);
-        if (person.getPhone() == null) {
-            phone.setText(NO_PHONE);
-        } else {
-            phone.setText(person.getPhone().value);
-        }
-        if (person.getEmail() == null) {
-            email.setText(NO_EMAIL);
-        } else {
-            email.setText(person.getEmail().value);
-        }
+        phone.setText(person.getPhone().toString());
+        email.setText(person.getEmail().toString());
         person.getTags().stream()
                 .sorted(Comparator.comparing(tag -> tag.tagName))
                 .forEach(tag -> tags.getChildren().add(new Label(tag.tagName)));

diff --git a/src/test/java/seedu/address/logic/parser/AddPersonCommandParserTest.java b/src/test/java/seedu/address/logic/parser/AddPersonCommandParserTest.java
@@ -78,7 +78,6 @@ public void parse_optionalFieldsMissing_success() {
         Person expectedPersonPhone = new PersonBuilder(AMY).withPhone(null).withTags().build();
         assertParseSuccess(parser, NAME_DESC_AMY + EMAIL_DESC_AMY,
                 new AddPersonCommand(expectedPersonPhone));
-
         // zero email
         Person expectedPersonEmail = new PersonBuilder(AMY).withEmail(null).withTags().build();
         assertParseSuccess(parser, NAME_DESC_AMY + PHONE_DESC_AMY,

diff --git a/src/test/java/seedu/address/logic/parser/EditPersonCommandParserTest.java b/src/test/java/seedu/address/logic/parser/EditPersonCommandParserTest.java
@@ -90,12 +90,6 @@ public void parse_invalidValue_failure() {
         // is tested at {@code parse_invalidValueFollowedByValidValue_success()}
         assertParseFailure(parser, "1" + PHONE_DESC_BOB + INVALID_PHONE_DESC, Phone.MESSAGE_CONSTRAINTS);
 
-        // while parsing {@code PREFIX_TAG} alone will reset the tags of the {@code Person} being edited,
-        // parsing it together with a valid tag results in error
-        assertParseFailure(parser, "1" + TAG_DESC_FRIEND + TAG_DESC_HUSBAND + TAG_EMPTY, Tag.MESSAGE_CONSTRAINTS);
-        assertParseFailure(parser, "1" + TAG_DESC_FRIEND + TAG_EMPTY + TAG_DESC_HUSBAND, Tag.MESSAGE_CONSTRAINTS);
-        assertParseFailure(parser, "1" + TAG_EMPTY + TAG_DESC_FRIEND + TAG_DESC_HUSBAND, Tag.MESSAGE_CONSTRAINTS);
-
         // multiple invalid values, but only the first invalid value is captured
         assertParseFailure(parser, "1" + INVALID_NAME_DESC + INVALID_EMAIL_DESC + VALID_ADDRESS_AMY + VALID_PHONE_AMY,
                 Name.MESSAGE_CONSTRAINTS);

diff --git a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java
@@ -3,6 +3,7 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static seedu.address.logic.parser.ParserUtil.MESSAGE_INVALID_INDEX;
+import static seedu.address.testutil.Assert.assertDoesNotThrow;
 import static seedu.address.testutil.Assert.assertThrows;
 import static seedu.address.testutil.TypicalIndexes.INDEX_FIRST_PERSON;
 
@@ -95,8 +96,8 @@ public void parsePhone_validValueWithWhitespace_returnsTrimmedPhone() throws Exc
     }
 
     @Test
-    public void parseEmail_null_throwsNullPointerException() {
-        assertThrows(NullPointerException.class, () -> ParserUtil.parseEmail((String) null));
+    public void parseEmail_null_doesNotThrowException() {
+        assertDoesNotThrow(() -> ParserUtil.parseEmail(null));
     }
 
     @Test
@@ -118,8 +119,8 @@ public void parseEmail_validValueWithWhitespace_returnsTrimmedEmail() throws Exc
     }
 
     @Test
-    public void parseTag_null_throwsNullPointerException() {
-        assertThrows(NullPointerException.class, () -> ParserUtil.parseTag(null));
+    public void parseTag_null_doesNotThrowException() {
+        assertDoesNotThrow(() -> ParserUtil.parseTag(null));
     }
 
     @Test

diff --git a/src/test/java/seedu/address/testutil/Assert.java b/src/test/java/seedu/address/testutil/Assert.java
@@ -31,4 +31,16 @@ public static void assertThrows(Class<? extends Throwable> expectedType, String
         Throwable thrownException = Assertions.assertThrows(expectedType, executable);
         Assertions.assertEquals(expectedMessage, thrownException.getMessage());
     }
+
+    /**
+     * Asserts that the {@code executable} does not throw any exceptions.
+     * This is a wrapper method that invokes {@link Assertions#assertDoesNotThrow(Executable)}, to maintain consistency
+     * with our custom {@link #assertThrows(Class, String, Executable)} method.
+     * To standardize API calls in this project, users should use this method instead of
+     * {@link Assertions#assertDoesNotThrow(Executable)}.
+     */
+    public static void assertDoesNotThrow(Executable executable) {
+        Assertions.assertDoesNotThrow(executable);
+    }
+
 }
diff --git a/src/test/java/seedu/address/testutil/InternshipBuilder.java b/src/test/java/seedu/address/testutil/InternshipBuilder.java
@@ -102,7 +102,11 @@ public InternshipBuilder withStatus(String status) {
      * Sets the {@code Interview Date} of the {@code Internship} that we are building.
      */
     public InternshipBuilder withInterviewDate(String date) {
-        this.interviewDate = new InterviewDate(date);
+        if (date == null || date.isBlank()) {
+            this.interviewDate = new InterviewDate(null);
+        } else {
+            this.interviewDate = new InterviewDate(date);
+        }
         return this;
     }