From 9940e181eb1155003556d7ed6a7c819999edc445 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Mon, 29 May 2017 22:16:53 +0800 Subject: [PATCH 01/20] TestUtil#assertThrows(...): support checked exceptions TestUtil#assertThrows(...) doesn't support functions which throw checked exceptions. For example, TestUtil.assertThrows(IllegalValueException.class, () -> { throw new IllegalValueException(""); }); would not compile, with the compiler complaining with: error: unreported exception IllegalValueException; must be caught or declared to be thrown This is because assertThrows(...) takes in a Runnable, whose run() method does not have `throws Exception` declared. java.util.concurrent.Callable, on the other hand, looks like what we want, as it is a functional interface whose call() method has `throws Exception` declared. Unfortunately, due to how Java's generics work, call() MUST return a value. This means we will need to tack on a `return null` at the end of our lambda bodies, which is too verbose. Not good. As such, let's define our own `VoidCallable` interface, and modify assertThrows(...) to take it. Like Callable, VoidCallable#call method has `throws Exceptions` declared, so our lambdas passed to assertThrows(...) can throw checked exceptions. Unlike Callable, its call() method doesn't need to return a return value, allowing us to write succinct lambda expressions to pass to assertThrows(...). --- src/test/java/seedu/address/testutil/Assert.java | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/test/java/seedu/address/testutil/Assert.java b/src/test/java/seedu/address/testutil/Assert.java index a6a7a1b529cf..17a2656fafda 100644 --- a/src/test/java/seedu/address/testutil/Assert.java +++ b/src/test/java/seedu/address/testutil/Assert.java @@ -8,11 +8,11 @@ public class Assert { /** - * Asserts that the {@code executable} throws the {@code expected} Exception. + * Asserts that the {@code callable} throws the {@code expected} Exception. */ - public static void assertThrows(Class expected, Runnable executable) { + public static void assertThrows(Class expected, VoidCallable callable) { try { - executable.run(); + callable.call(); } catch (Throwable actualException) { if (actualException.getClass().isAssignableFrom(expected)) { return; @@ -24,4 +24,13 @@ public static void assertThrows(Class expected, Runnable ex throw new AssertionFailedError( String.format("Expected %s to be thrown, but nothing was thrown.", expected.getName())); } + + /** + * Represents a function which does not return anything and may throw an exception. + */ + @FunctionalInterface + public interface VoidCallable { + void call() throws Exception; + } + } From de4a334c8e760f2757524f5e2e49e665023d911e Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Mon, 4 Dec 2017 14:48:52 +0800 Subject: [PATCH 02/20] NameTest, PhoneTest, EmailTest, AddressTest, TagTest: add missing null tests The constructors and isValidX() methods of these classes would throw NullPointerExceptions on null inputs, but this behavior is not tested. Let's add tests for them. --- .../address/model/person/AddressTest.java | 10 ++++++++++ .../seedu/address/model/person/EmailTest.java | 10 ++++++++++ .../seedu/address/model/person/NameTest.java | 10 ++++++++++ .../seedu/address/model/person/PhoneTest.java | 10 ++++++++++ .../java/seedu/address/model/tag/TagTest.java | 20 +++++++++++++++++++ 5 files changed, 60 insertions(+) create mode 100644 src/test/java/seedu/address/model/tag/TagTest.java diff --git a/src/test/java/seedu/address/model/person/AddressTest.java b/src/test/java/seedu/address/model/person/AddressTest.java index d54d9299ad5e..4c02e886ddf1 100644 --- a/src/test/java/seedu/address/model/person/AddressTest.java +++ b/src/test/java/seedu/address/model/person/AddressTest.java @@ -5,10 +5,20 @@ import org.junit.Test; +import seedu.address.testutil.Assert; + public class AddressTest { + @Test + public void constructor_null_throwsNullPointerException() { + Assert.assertThrows(NullPointerException.class, () -> new Address(null)); + } + @Test public void isValidAddress() { + // null address + Assert.assertThrows(NullPointerException.class, () -> Address.isValidAddress(null)); + // invalid addresses assertFalse(Address.isValidAddress("")); // empty string assertFalse(Address.isValidAddress(" ")); // spaces only diff --git a/src/test/java/seedu/address/model/person/EmailTest.java b/src/test/java/seedu/address/model/person/EmailTest.java index 087192632acc..1d62e1b5772a 100644 --- a/src/test/java/seedu/address/model/person/EmailTest.java +++ b/src/test/java/seedu/address/model/person/EmailTest.java @@ -5,10 +5,20 @@ import org.junit.Test; +import seedu.address.testutil.Assert; + public class EmailTest { + @Test + public void constructor_null_throwsNullPointerException() { + Assert.assertThrows(NullPointerException.class, () -> new Email(null)); + } + @Test public void isValidEmail() { + // null email + Assert.assertThrows(NullPointerException.class, () -> Email.isValidEmail(null)); + // blank email assertFalse(Email.isValidEmail("")); // empty string assertFalse(Email.isValidEmail(" ")); // spaces only diff --git a/src/test/java/seedu/address/model/person/NameTest.java b/src/test/java/seedu/address/model/person/NameTest.java index 0dcbc7f82812..4541262c6528 100644 --- a/src/test/java/seedu/address/model/person/NameTest.java +++ b/src/test/java/seedu/address/model/person/NameTest.java @@ -5,10 +5,20 @@ import org.junit.Test; +import seedu.address.testutil.Assert; + public class NameTest { + @Test + public void constructor_null_throwsNullPointerException() { + Assert.assertThrows(NullPointerException.class, () -> new Name(null)); + } + @Test public void isValidName() { + // null name + Assert.assertThrows(NullPointerException.class, () -> Name.isValidName(null)); + // invalid name assertFalse(Name.isValidName("")); // empty string assertFalse(Name.isValidName(" ")); // spaces only diff --git a/src/test/java/seedu/address/model/person/PhoneTest.java b/src/test/java/seedu/address/model/person/PhoneTest.java index 8e65e7c91756..a4680a2054c4 100644 --- a/src/test/java/seedu/address/model/person/PhoneTest.java +++ b/src/test/java/seedu/address/model/person/PhoneTest.java @@ -5,10 +5,20 @@ import org.junit.Test; +import seedu.address.testutil.Assert; + public class PhoneTest { + @Test + public void constructor_null_throwsNullPointerException() { + Assert.assertThrows(NullPointerException.class, () -> new Phone(null)); + } + @Test public void isValidPhone() { + // null phone number + Assert.assertThrows(NullPointerException.class, () -> Phone.isValidPhone(null)); + // invalid phone numbers assertFalse(Phone.isValidPhone("")); // empty string assertFalse(Phone.isValidPhone(" ")); // spaces only diff --git a/src/test/java/seedu/address/model/tag/TagTest.java b/src/test/java/seedu/address/model/tag/TagTest.java new file mode 100644 index 000000000000..38c5a7be1a40 --- /dev/null +++ b/src/test/java/seedu/address/model/tag/TagTest.java @@ -0,0 +1,20 @@ +package seedu.address.model.tag; + +import org.junit.Test; + +import seedu.address.testutil.Assert; + +public class TagTest { + + @Test + public void constructor_null_throwsNullPointerException() { + Assert.assertThrows(NullPointerException.class, () -> new Tag(null)); + } + + @Test + public void isValidTagName() { + // null tag name + Assert.assertThrows(NullPointerException.class, () -> Tag.isValidTagName(null)); + } + +} From c07b567c6bd97c4d842cdec777a8943ef29cea44 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Tue, 5 Dec 2017 15:00:27 +0800 Subject: [PATCH 03/20] XmlAdaptedPersonTest: add tests for #toModelType() XmlAdaptedPersonTest#toModelType() doesn't have any dedicated tests. Let's add some. This is mainly done to prevent code coverage drops when we tweak XmlAdaptedPersonTest#toModelType() in later commits. --- .../address/storage/XmlAdaptedPerson.java | 12 +++ .../seedu/address/storage/XmlAdaptedTag.java | 7 ++ .../address/storage/XmlAdaptedPersonTest.java | 74 +++++++++++++++++++ 3 files changed, 93 insertions(+) create mode 100644 src/test/java/seedu/address/storage/XmlAdaptedPersonTest.java diff --git a/src/main/java/seedu/address/storage/XmlAdaptedPerson.java b/src/main/java/seedu/address/storage/XmlAdaptedPerson.java index 3332430bffa5..fc631df1435b 100644 --- a/src/main/java/seedu/address/storage/XmlAdaptedPerson.java +++ b/src/main/java/seedu/address/storage/XmlAdaptedPerson.java @@ -39,6 +39,18 @@ public class XmlAdaptedPerson { */ public XmlAdaptedPerson() {} + /** + * Constructs an {@code XmlAdaptedPerson} with the given person details. + */ + public XmlAdaptedPerson(String name, String phone, String email, String address, List tagged) { + this.name = name; + this.phone = phone; + this.email = email; + this.address = address; + if (tagged != null) { + this.tagged = new ArrayList<>(tagged); + } + } /** * Converts a given Person into this class for JAXB use. diff --git a/src/main/java/seedu/address/storage/XmlAdaptedTag.java b/src/main/java/seedu/address/storage/XmlAdaptedTag.java index aad2d96dde3f..f7775fcd98f2 100644 --- a/src/main/java/seedu/address/storage/XmlAdaptedTag.java +++ b/src/main/java/seedu/address/storage/XmlAdaptedTag.java @@ -19,6 +19,13 @@ public class XmlAdaptedTag { */ public XmlAdaptedTag() {} + /** + * Constructs a {@code XmlAdaptedTag} with the given {@code tagName}. + */ + public XmlAdaptedTag(String tagName) { + this.tagName = tagName; + } + /** * Converts a given Tag into this class for JAXB use. * diff --git a/src/test/java/seedu/address/storage/XmlAdaptedPersonTest.java b/src/test/java/seedu/address/storage/XmlAdaptedPersonTest.java new file mode 100644 index 000000000000..e38b88839884 --- /dev/null +++ b/src/test/java/seedu/address/storage/XmlAdaptedPersonTest.java @@ -0,0 +1,74 @@ +package seedu.address.storage; + +import static org.junit.Assert.assertEquals; +import static seedu.address.testutil.TypicalPersons.BENSON; + +import java.util.ArrayList; +import java.util.List; +import java.util.stream.Collectors; + +import org.junit.Test; + +import seedu.address.commons.exceptions.IllegalValueException; +import seedu.address.testutil.Assert; + +public class XmlAdaptedPersonTest { + private static final String INVALID_NAME = "R@chel"; + private static final String INVALID_PHONE = "+651234"; + private static final String INVALID_ADDRESS = " "; + private static final String INVALID_EMAIL = "example.com"; + private static final String INVALID_TAG = "#friend"; + + private static final String VALID_NAME = BENSON.getName().toString(); + private static final String VALID_PHONE = BENSON.getPhone().toString(); + private static final String VALID_EMAIL = BENSON.getEmail().toString(); + private static final String VALID_ADDRESS = BENSON.getAddress().toString(); + + private final List validTags = BENSON.getTags().stream() + .map(tag -> new XmlAdaptedTag(tag)) + .collect(Collectors.toList()); + + @Test + public void toModelType_validPersonDetails_returnsPerson() throws Exception { + XmlAdaptedPerson person = new XmlAdaptedPerson(BENSON); + assertEquals(BENSON, person.toModelType()); + } + + @Test + public void toModelType_invalidName_throwsIllegalValueException() { + XmlAdaptedPerson person = + new XmlAdaptedPerson(INVALID_NAME, VALID_PHONE, VALID_EMAIL, VALID_ADDRESS, validTags); + Assert.assertThrows(IllegalValueException.class, () -> person.toModelType()); + } + + @Test + public void toModelType_invalidPhone_throwsIllegalValueException() { + XmlAdaptedPerson person = + new XmlAdaptedPerson(VALID_NAME, INVALID_PHONE, VALID_EMAIL, VALID_ADDRESS, validTags); + Assert.assertThrows(IllegalValueException.class, () -> person.toModelType()); + } + + @Test + public void toModelType_invalidEmail_throwsIllegalValueException() { + XmlAdaptedPerson person = + new XmlAdaptedPerson(VALID_NAME, VALID_PHONE, INVALID_EMAIL, VALID_ADDRESS, validTags); + Assert.assertThrows(IllegalValueException.class, () -> person.toModelType()); + } + + @Test + public void toModelType_invalidAddress_throwsIllegalValueException() { + XmlAdaptedPerson person = + new XmlAdaptedPerson(VALID_NAME, VALID_PHONE, VALID_EMAIL, INVALID_ADDRESS, validTags); + Assert.assertThrows(IllegalValueException.class, () -> person.toModelType()); + } + + @Test + public void toModelType_invalidTags_throwsIllegalValueException() { + List invalidTags = new ArrayList<>(validTags); + invalidTags.add(new XmlAdaptedTag(INVALID_TAG)); + XmlAdaptedPerson person = + new XmlAdaptedPerson(VALID_NAME, VALID_PHONE, VALID_EMAIL, VALID_ADDRESS, invalidTags); + Assert.assertThrows(IllegalValueException.class, () -> person.toModelType()); + } + +} From b20dc8f6530951a7cb97524f8513eab84df3f846 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Mon, 29 May 2017 22:07:19 +0800 Subject: [PATCH 04/20] Move string trimming responsibility from Name to ParserUtil#parseName(String) Name's constructor modifies the input string by trimming leading and trailing whitespace. However, given that the model component's sole responsibility is to store data in memory, it shouldn't be modifying any input. Rather, any munging of input should be done in the parser layer. Fix this by moving the string trimming logic from Name to ParserUtil#parseName(String). --- .../address/logic/parser/ParserUtil.java | 14 +++++++++- .../java/seedu/address/model/person/Name.java | 5 ++-- .../address/logic/parser/ParserUtilTest.java | 26 +++++++++++++------ 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/src/main/java/seedu/address/logic/parser/ParserUtil.java b/src/main/java/seedu/address/logic/parser/ParserUtil.java index 2ca577e9e0e6..f8a56597548f 100644 --- a/src/main/java/seedu/address/logic/parser/ParserUtil.java +++ b/src/main/java/seedu/address/logic/parser/ParserUtil.java @@ -43,13 +43,25 @@ public static Index parseIndex(String oneBasedIndex) throws IllegalValueExceptio return Index.fromOneBased(Integer.parseInt(trimmedIndex)); } + /** + * Parses a {@code String name} into a {@code Name}. + * Leading and trailing whitespaces will be trimmed. + * + * @throws IllegalValueException if the given {@code name} is invalid. + */ + public static Name parseName(String name) throws IllegalValueException { + requireNonNull(name); + String trimmedName = name.trim(); + return new Name(trimmedName); + } + /** * Parses a {@code Optional name} into an {@code Optional} if {@code name} is present. * See header comment of this class regarding the use of {@code Optional} parameters. */ public static Optional parseName(Optional name) throws IllegalValueException { requireNonNull(name); - return name.isPresent() ? Optional.of(new Name(name.get())) : Optional.empty(); + return name.isPresent() ? Optional.of(parseName(name.get())) : Optional.empty(); } /** diff --git a/src/main/java/seedu/address/model/person/Name.java b/src/main/java/seedu/address/model/person/Name.java index 5d138e8ba223..aba756ecb24f 100644 --- a/src/main/java/seedu/address/model/person/Name.java +++ b/src/main/java/seedu/address/model/person/Name.java @@ -28,11 +28,10 @@ public class Name { */ public Name(String name) throws IllegalValueException { requireNonNull(name); - String trimmedName = name.trim(); - if (!isValidName(trimmedName)) { + if (!isValidName(name)) { throw new IllegalValueException(MESSAGE_NAME_CONSTRAINTS); } - this.fullName = trimmedName; + this.fullName = name; } /** diff --git a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java index 03d839cf1064..da5f30a07768 100644 --- a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java +++ b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java @@ -23,6 +23,7 @@ import seedu.address.model.person.Name; import seedu.address.model.person.Phone; import seedu.address.model.tag.Tag; +import seedu.address.testutil.Assert; public class ParserUtilTest { private static final String INVALID_NAME = "R@chel"; @@ -38,6 +39,8 @@ public class ParserUtilTest { private static final String VALID_TAG_1 = "friend"; private static final String VALID_TAG_2 = "neighbour"; + private static final String WHITESPACE = " \t\r\n"; + @Rule public final ExpectedException thrown = ExpectedException.none(); @@ -64,15 +67,15 @@ public void parseIndex_validInput_success() throws Exception { } @Test - public void parseName_null_throwsNullPointerException() throws Exception { - thrown.expect(NullPointerException.class); - ParserUtil.parseName(null); + public void parseName_null_throwsNullPointerException() { + Assert.assertThrows(NullPointerException.class, () -> ParserUtil.parseName((String) null)); + Assert.assertThrows(NullPointerException.class, () -> ParserUtil.parseName((Optional) null)); } @Test - public void parseName_invalidValue_throwsIllegalValueException() throws Exception { - thrown.expect(IllegalValueException.class); - ParserUtil.parseName(Optional.of(INVALID_NAME)); + public void parseName_invalidValue_throwsIllegalValueException() { + Assert.assertThrows(IllegalValueException.class, () -> ParserUtil.parseName(INVALID_NAME)); + Assert.assertThrows(IllegalValueException.class, () -> ParserUtil.parseName(Optional.of(INVALID_NAME))); } @Test @@ -83,9 +86,16 @@ public void parseName_optionalEmpty_returnsOptionalEmpty() throws Exception { @Test public void parseName_validValue_returnsName() throws Exception { Name expectedName = new Name(VALID_NAME); - Optional actualName = ParserUtil.parseName(Optional.of(VALID_NAME)); + assertEquals(expectedName, ParserUtil.parseName(VALID_NAME)); + assertEquals(Optional.of(expectedName), ParserUtil.parseName(Optional.of(VALID_NAME))); + } - assertEquals(expectedName, actualName.get()); + @Test + public void parseName_validValueWithWhitespace_returnsTrimmedName() throws Exception { + String nameWithWhitespace = WHITESPACE + VALID_NAME + WHITESPACE; + Name expectedName = new Name(VALID_NAME); + assertEquals(expectedName, ParserUtil.parseName(nameWithWhitespace)); + assertEquals(Optional.of(expectedName), ParserUtil.parseName(Optional.of(nameWithWhitespace))); } @Test From 0f0ca2c494d0c93782fa21d68efc6955616d7e19 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Tue, 30 May 2017 17:55:43 +0800 Subject: [PATCH 05/20] Move Name validation step from model to the parser and storage layers The Name(String) constructor validates the passed in name String, throwing a checked IllegalValueException error if the input fails validation. However, according to our architecture, the model layer should only be concerned with the storage of data in memory. Any data which reaches the model layer should have already been validated. As such, if for any reason the input name String fails validation in the model layer, it is most likely a programmer error, and a runtime exception should thus be thrown instead. Fix this by moving the input validation step, and the throwing of the checked IllegalValueException, from the Name constructor to the parser layer and storage layers. The Name constructor will instead use the checkArgument(...) helper function to verify the "valid name" precondition. Thanks to this fix, as Name's constructor does not throw checked exceptions any more, we can remove a try/catch block from PersonBuilder as well (otherwise the code won't compile). --- .../java/seedu/address/logic/parser/ParserUtil.java | 3 +++ src/main/java/seedu/address/model/person/Name.java | 13 +++++-------- .../seedu/address/storage/XmlAdaptedPerson.java | 4 ++++ .../java/seedu/address/model/person/NameTest.java | 6 ++++++ .../java/seedu/address/testutil/PersonBuilder.java | 6 +----- 5 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/main/java/seedu/address/logic/parser/ParserUtil.java b/src/main/java/seedu/address/logic/parser/ParserUtil.java index f8a56597548f..73bda2501d34 100644 --- a/src/main/java/seedu/address/logic/parser/ParserUtil.java +++ b/src/main/java/seedu/address/logic/parser/ParserUtil.java @@ -52,6 +52,9 @@ public static Index parseIndex(String oneBasedIndex) throws IllegalValueExceptio public static Name parseName(String name) throws IllegalValueException { requireNonNull(name); String trimmedName = name.trim(); + if (!Name.isValidName(trimmedName)) { + throw new IllegalValueException(Name.MESSAGE_NAME_CONSTRAINTS); + } return new Name(trimmedName); } diff --git a/src/main/java/seedu/address/model/person/Name.java b/src/main/java/seedu/address/model/person/Name.java index aba756ecb24f..8e632943c4cf 100644 --- a/src/main/java/seedu/address/model/person/Name.java +++ b/src/main/java/seedu/address/model/person/Name.java @@ -1,8 +1,7 @@ package seedu.address.model.person; import static java.util.Objects.requireNonNull; - -import seedu.address.commons.exceptions.IllegalValueException; +import static seedu.address.commons.util.AppUtil.checkArgument; /** * Represents a Person's name in the address book. @@ -22,15 +21,13 @@ public class Name { public final String fullName; /** - * Validates given name. + * Constructs a {@code Name}. * - * @throws IllegalValueException if given name string is invalid. + * @param name A valid name. */ - public Name(String name) throws IllegalValueException { + public Name(String name) { requireNonNull(name); - if (!isValidName(name)) { - throw new IllegalValueException(MESSAGE_NAME_CONSTRAINTS); - } + checkArgument(isValidName(name), MESSAGE_NAME_CONSTRAINTS); this.fullName = name; } diff --git a/src/main/java/seedu/address/storage/XmlAdaptedPerson.java b/src/main/java/seedu/address/storage/XmlAdaptedPerson.java index fc631df1435b..00bef16542da 100644 --- a/src/main/java/seedu/address/storage/XmlAdaptedPerson.java +++ b/src/main/java/seedu/address/storage/XmlAdaptedPerson.java @@ -78,6 +78,10 @@ public Person toModelType() throws IllegalValueException { for (XmlAdaptedTag tag : tagged) { personTags.add(tag.toModelType()); } + + if (!Name.isValidName(this.name)) { + throw new IllegalValueException(Name.MESSAGE_NAME_CONSTRAINTS); + } final Name name = new Name(this.name); final Phone phone = new Phone(this.phone); final Email email = new Email(this.email); diff --git a/src/test/java/seedu/address/model/person/NameTest.java b/src/test/java/seedu/address/model/person/NameTest.java index 4541262c6528..b4a356b6f011 100644 --- a/src/test/java/seedu/address/model/person/NameTest.java +++ b/src/test/java/seedu/address/model/person/NameTest.java @@ -14,6 +14,12 @@ public void constructor_null_throwsNullPointerException() { Assert.assertThrows(NullPointerException.class, () -> new Name(null)); } + @Test + public void constructor_invalidName_throwsIllegalArgumentException() { + String invalidName = ""; + Assert.assertThrows(IllegalArgumentException.class, () -> new Name(invalidName)); + } + @Test public void isValidName() { // null name diff --git a/src/test/java/seedu/address/testutil/PersonBuilder.java b/src/test/java/seedu/address/testutil/PersonBuilder.java index 800d20bb56fe..ab54ee5e47dc 100644 --- a/src/test/java/seedu/address/testutil/PersonBuilder.java +++ b/src/test/java/seedu/address/testutil/PersonBuilder.java @@ -49,11 +49,7 @@ public PersonBuilder(ReadOnlyPerson personToCopy) { * Sets the {@code Name} of the {@code Person} that we are building. */ public PersonBuilder withName(String name) { - try { - this.person.setName(new Name(name)); - } catch (IllegalValueException ive) { - throw new IllegalArgumentException("name is expected to be unique."); - } + this.person.setName(new Name(name)); return this; } From b3020230a2b309058dd847ba0385574b36dc5052 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Tue, 30 May 2017 18:03:35 +0800 Subject: [PATCH 06/20] Move string trimming responsibility from Phone to ParserUtil#parsePhone(String) Phone's constructor modifies the input string by trimming leading and trailing whitespace. However, given that the model component's sole responsibility is to store data in memory, it shouldn't be modifying any input. Rather, any munging of input should be done in the parser layer. Fix this by moving the string trimming logic from Phone to ParserUtil#parsePhone(String). --- .../address/logic/parser/ParserUtil.java | 14 ++++++++++- .../seedu/address/model/person/Phone.java | 5 ++-- .../address/logic/parser/ParserUtilTest.java | 23 ++++++++++++------- 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/src/main/java/seedu/address/logic/parser/ParserUtil.java b/src/main/java/seedu/address/logic/parser/ParserUtil.java index 73bda2501d34..5d94b5418a33 100644 --- a/src/main/java/seedu/address/logic/parser/ParserUtil.java +++ b/src/main/java/seedu/address/logic/parser/ParserUtil.java @@ -67,13 +67,25 @@ public static Optional parseName(Optional name) throws IllegalValu return name.isPresent() ? Optional.of(parseName(name.get())) : Optional.empty(); } + /** + * Parses a {@code String phone} into a {@code Phone}. + * Leading and trailing whitespaces will be trimmed. + * + * @throws IllegalValueException if the given {@code phone} is invalid. + */ + public static Phone parsePhone(String phone) throws IllegalValueException { + requireNonNull(phone); + String trimmedPhone = phone.trim(); + return new Phone(trimmedPhone); + } + /** * Parses a {@code Optional phone} into an {@code Optional} if {@code phone} is present. * See header comment of this class regarding the use of {@code Optional} parameters. */ public static Optional parsePhone(Optional phone) throws IllegalValueException { requireNonNull(phone); - return phone.isPresent() ? Optional.of(new Phone(phone.get())) : Optional.empty(); + return phone.isPresent() ? Optional.of(parsePhone(phone.get())) : Optional.empty(); } /** diff --git a/src/main/java/seedu/address/model/person/Phone.java b/src/main/java/seedu/address/model/person/Phone.java index 3a56110b468a..c0b9766934cb 100644 --- a/src/main/java/seedu/address/model/person/Phone.java +++ b/src/main/java/seedu/address/model/person/Phone.java @@ -23,11 +23,10 @@ public class Phone { */ public Phone(String phone) throws IllegalValueException { requireNonNull(phone); - String trimmedPhone = phone.trim(); - if (!isValidPhone(trimmedPhone)) { + if (!isValidPhone(phone)) { throw new IllegalValueException(MESSAGE_PHONE_CONSTRAINTS); } - this.value = trimmedPhone; + this.value = phone; } /** diff --git a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java index da5f30a07768..6c7778beb012 100644 --- a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java +++ b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java @@ -99,15 +99,15 @@ public void parseName_validValueWithWhitespace_returnsTrimmedName() throws Excep } @Test - public void parsePhone_null_throwsNullPointerException() throws Exception { - thrown.expect(NullPointerException.class); - ParserUtil.parsePhone(null); + public void parsePhone_null_throwsNullPointerException() { + Assert.assertThrows(NullPointerException.class, () -> ParserUtil.parsePhone((String) null)); + Assert.assertThrows(NullPointerException.class, () -> ParserUtil.parsePhone((Optional) null)); } @Test - public void parsePhone_invalidValue_throwsIllegalValueException() throws Exception { - thrown.expect(IllegalValueException.class); - ParserUtil.parsePhone(Optional.of(INVALID_PHONE)); + public void parsePhone_invalidValue_throwsIllegalValueException() { + Assert.assertThrows(IllegalValueException.class, () -> ParserUtil.parsePhone(INVALID_PHONE)); + Assert.assertThrows(IllegalValueException.class, () -> ParserUtil.parsePhone(Optional.of(INVALID_PHONE))); } @Test @@ -118,9 +118,16 @@ public void parsePhone_optionalEmpty_returnsOptionalEmpty() throws Exception { @Test public void parsePhone_validValue_returnsPhone() throws Exception { Phone expectedPhone = new Phone(VALID_PHONE); - Optional actualPhone = ParserUtil.parsePhone(Optional.of(VALID_PHONE)); + assertEquals(expectedPhone, ParserUtil.parsePhone(VALID_PHONE)); + assertEquals(Optional.of(expectedPhone), ParserUtil.parsePhone(Optional.of(VALID_PHONE))); + } - assertEquals(expectedPhone, actualPhone.get()); + @Test + public void parsePhone_validValueWithWhitespace_returnsPhone() throws Exception { + String phoneWithWhitespace = WHITESPACE + VALID_PHONE + WHITESPACE; + Phone expectedPhone = new Phone(VALID_PHONE); + assertEquals(expectedPhone, ParserUtil.parsePhone(phoneWithWhitespace)); + assertEquals(Optional.of(expectedPhone), ParserUtil.parsePhone(Optional.of(phoneWithWhitespace))); } @Test From d5871fca43f9aee7c721244bb51fe4ca4988f8b2 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Tue, 30 May 2017 18:08:52 +0800 Subject: [PATCH 07/20] Move Phone validation step from model to parser and storage layers The Phone(String) constructor validates the passed in phone String, throwing a checked IllegalValueException error if the input fails validation. However, according to our architecture, the model layer should only be concerned with the storage of data in memory. Any data which reaches the model layer should have already been validated. As such, if for any reason the input phone String fails validation in the model layer, it is most likely a programmer error, and a runtime exception should thus be thrown instead. Fix this by moving the input validation step, and the throwing of the checked IllegalValueException, from the Phone constructor to the parser and storage layers. The Phone constructor will instead use the checkArgument(...) helper function to verify the "valid phone format" precondition. Thanks to this fix, as Phone's constructor does not throw checked exceptions any more, we can remove a try/catch block from PersonBuilder as well (otherwise the code won't compile). --- .../java/seedu/address/logic/parser/ParserUtil.java | 3 +++ src/main/java/seedu/address/model/person/Phone.java | 13 +++++-------- .../seedu/address/storage/XmlAdaptedPerson.java | 4 ++++ .../java/seedu/address/model/person/PhoneTest.java | 6 ++++++ .../java/seedu/address/testutil/PersonBuilder.java | 6 +----- 5 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/main/java/seedu/address/logic/parser/ParserUtil.java b/src/main/java/seedu/address/logic/parser/ParserUtil.java index 5d94b5418a33..de59a5d13d6a 100644 --- a/src/main/java/seedu/address/logic/parser/ParserUtil.java +++ b/src/main/java/seedu/address/logic/parser/ParserUtil.java @@ -76,6 +76,9 @@ public static Optional parseName(Optional name) throws IllegalValu public static Phone parsePhone(String phone) throws IllegalValueException { requireNonNull(phone); String trimmedPhone = phone.trim(); + if (!Phone.isValidPhone(trimmedPhone)) { + throw new IllegalValueException(Phone.MESSAGE_PHONE_CONSTRAINTS); + } return new Phone(trimmedPhone); } diff --git a/src/main/java/seedu/address/model/person/Phone.java b/src/main/java/seedu/address/model/person/Phone.java index c0b9766934cb..11b5435ac247 100644 --- a/src/main/java/seedu/address/model/person/Phone.java +++ b/src/main/java/seedu/address/model/person/Phone.java @@ -1,8 +1,7 @@ package seedu.address.model.person; import static java.util.Objects.requireNonNull; - -import seedu.address.commons.exceptions.IllegalValueException; +import static seedu.address.commons.util.AppUtil.checkArgument; /** * Represents a Person's phone number in the address book. @@ -17,15 +16,13 @@ public class Phone { public final String value; /** - * Validates given phone number. + * Constructs a {@code Phone}. * - * @throws IllegalValueException if given phone string is invalid. + * @param phone A valid phone number. */ - public Phone(String phone) throws IllegalValueException { + public Phone(String phone) { requireNonNull(phone); - if (!isValidPhone(phone)) { - throw new IllegalValueException(MESSAGE_PHONE_CONSTRAINTS); - } + checkArgument(isValidPhone(phone), MESSAGE_PHONE_CONSTRAINTS); this.value = phone; } diff --git a/src/main/java/seedu/address/storage/XmlAdaptedPerson.java b/src/main/java/seedu/address/storage/XmlAdaptedPerson.java index 00bef16542da..48bddccc4b7a 100644 --- a/src/main/java/seedu/address/storage/XmlAdaptedPerson.java +++ b/src/main/java/seedu/address/storage/XmlAdaptedPerson.java @@ -83,6 +83,10 @@ public Person toModelType() throws IllegalValueException { throw new IllegalValueException(Name.MESSAGE_NAME_CONSTRAINTS); } final Name name = new Name(this.name); + + if (!Phone.isValidPhone(this.phone)) { + throw new IllegalValueException(Phone.MESSAGE_PHONE_CONSTRAINTS); + } final Phone phone = new Phone(this.phone); final Email email = new Email(this.email); final Address address = new Address(this.address); diff --git a/src/test/java/seedu/address/model/person/PhoneTest.java b/src/test/java/seedu/address/model/person/PhoneTest.java index a4680a2054c4..c721cbbfc048 100644 --- a/src/test/java/seedu/address/model/person/PhoneTest.java +++ b/src/test/java/seedu/address/model/person/PhoneTest.java @@ -14,6 +14,12 @@ public void constructor_null_throwsNullPointerException() { Assert.assertThrows(NullPointerException.class, () -> new Phone(null)); } + @Test + public void constructor_invalidPhone_throwsIllegalArgumentException() { + String invalidPhone = ""; + Assert.assertThrows(IllegalArgumentException.class, () -> new Phone(invalidPhone)); + } + @Test public void isValidPhone() { // null phone number diff --git a/src/test/java/seedu/address/testutil/PersonBuilder.java b/src/test/java/seedu/address/testutil/PersonBuilder.java index ab54ee5e47dc..fe58a46c2981 100644 --- a/src/test/java/seedu/address/testutil/PersonBuilder.java +++ b/src/test/java/seedu/address/testutil/PersonBuilder.java @@ -81,11 +81,7 @@ public PersonBuilder withAddress(String address) { * Sets the {@code Phone} of the {@code Person} that we are building. */ public PersonBuilder withPhone(String phone) { - try { - this.person.setPhone(new Phone(phone)); - } catch (IllegalValueException ive) { - throw new IllegalArgumentException("phone is expected to be unique."); - } + this.person.setPhone(new Phone(phone)); return this; } From d0b2451dca0d1ba36a5d54dac793ac1e1ac08987 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Tue, 30 May 2017 18:14:18 +0800 Subject: [PATCH 08/20] Move string trimming responsibility from Address to ParserUtil#parseAddress(String) Address's constructor modifies the input string by trimming leading and trailing whitespace. However, given that the model component's sole responsibility is to store data in memory, it shouldn't be modifying any input. Rather, any munging of input should be done in the parser layer. Fix this by moving the string trimming logic from Address to ParserUtil#parseAddress(String). --- .../address/logic/parser/ParserUtil.java | 14 ++++++++++- .../address/logic/parser/ParserUtilTest.java | 23 ++++++++++++------- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/src/main/java/seedu/address/logic/parser/ParserUtil.java b/src/main/java/seedu/address/logic/parser/ParserUtil.java index de59a5d13d6a..319b6f3aa315 100644 --- a/src/main/java/seedu/address/logic/parser/ParserUtil.java +++ b/src/main/java/seedu/address/logic/parser/ParserUtil.java @@ -91,13 +91,25 @@ public static Optional parsePhone(Optional phone) throws IllegalV return phone.isPresent() ? Optional.of(parsePhone(phone.get())) : Optional.empty(); } + /** + * Parses a {@code String address} into an {@code Address}. + * Leading and trailing whitespaces will be trimmed. + * + * @throws IllegalValueException if the given {@code address} is invalid. + */ + public static Address parseAddress(String address) throws IllegalValueException { + requireNonNull(address); + String trimmedAddress = address.trim(); + return new Address(trimmedAddress); + } + /** * Parses a {@code Optional address} into an {@code Optional
} if {@code address} is present. * See header comment of this class regarding the use of {@code Optional} parameters. */ public static Optional
parseAddress(Optional address) throws IllegalValueException { requireNonNull(address); - return address.isPresent() ? Optional.of(new Address(address.get())) : Optional.empty(); + return address.isPresent() ? Optional.of(parseAddress(address.get())) : Optional.empty(); } /** diff --git a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java index 6c7778beb012..3f379c75b3ef 100644 --- a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java +++ b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java @@ -131,15 +131,15 @@ public void parsePhone_validValueWithWhitespace_returnsPhone() throws Exception } @Test - public void parseAddress_null_throwsNullPointerException() throws Exception { - thrown.expect(NullPointerException.class); - ParserUtil.parseAddress(null); + public void parseAddress_null_throwsNullPointerException() { + Assert.assertThrows(NullPointerException.class, () -> ParserUtil.parseAddress((String) null)); + Assert.assertThrows(NullPointerException.class, () -> ParserUtil.parseAddress((Optional) null)); } @Test - public void parseAddress_invalidValue_throwsIllegalValueException() throws Exception { - thrown.expect(IllegalValueException.class); - ParserUtil.parseAddress(Optional.of(INVALID_ADDRESS)); + public void parseAddress_invalidValue_throwsIllegalValueException() { + Assert.assertThrows(IllegalValueException.class, () -> ParserUtil.parseAddress(INVALID_ADDRESS)); + Assert.assertThrows(IllegalValueException.class, () -> ParserUtil.parseAddress(Optional.of(INVALID_ADDRESS))); } @Test @@ -150,9 +150,16 @@ public void parseAddress_optionalEmpty_returnsOptionalEmpty() throws Exception { @Test public void parseAddress_validValue_returnsAddress() throws Exception { Address expectedAddress = new Address(VALID_ADDRESS); - Optional
actualAddress = ParserUtil.parseAddress(Optional.of(VALID_ADDRESS)); + assertEquals(expectedAddress, ParserUtil.parseAddress(VALID_ADDRESS)); + assertEquals(Optional.of(expectedAddress), ParserUtil.parseAddress(Optional.of(VALID_ADDRESS))); + } - assertEquals(expectedAddress, actualAddress.get()); + @Test + public void parseAddress_validValueWithWhitespace_returnsAddress() throws Exception { + String addressWithWhitespace = WHITESPACE + VALID_ADDRESS + WHITESPACE; + Address expectedAddress = new Address(VALID_ADDRESS); + assertEquals(expectedAddress, ParserUtil.parseAddress(addressWithWhitespace)); + assertEquals(Optional.of(expectedAddress), ParserUtil.parseAddress(Optional.of(addressWithWhitespace))); } @Test From 033a88b4cf97e4e5b1106a6995f3a85a7598abf6 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Tue, 30 May 2017 18:33:05 +0800 Subject: [PATCH 09/20] Move Address validation step from model to parser and storage layers The Address(String) constructor validates the passed in address String, throwing a checked IllegalValueException error if the input fails validation. However, according to our architecture, the model layer should only be concerned with the storage of data in memory. Any data which reaches the model layer should have already been validated. As such, if for any reason the input address String fails validation in the model layer, it is most likely a programmer error, and a runtime exception should thus be thrown instead. Fix this by moving the input validation step, and the throwing of the checked IllegalValueException, from the Address constructor to the parser and storage layers. The Address constructor will instead use the checkArgument(...) helper function to verify the "valid address format" precondition. Thanks to this fix, as Address's constructor does not throw checked exceptions any more, we can remove a try/catch block from PersonBuilder as well (otherwise the code won't compile). --- .../java/seedu/address/logic/parser/ParserUtil.java | 3 +++ .../java/seedu/address/model/person/Address.java | 13 +++++-------- .../seedu/address/storage/XmlAdaptedPerson.java | 5 +++++ .../seedu/address/model/person/AddressTest.java | 6 ++++++ .../java/seedu/address/testutil/PersonBuilder.java | 6 +----- 5 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/main/java/seedu/address/logic/parser/ParserUtil.java b/src/main/java/seedu/address/logic/parser/ParserUtil.java index 319b6f3aa315..f6ac4d033001 100644 --- a/src/main/java/seedu/address/logic/parser/ParserUtil.java +++ b/src/main/java/seedu/address/logic/parser/ParserUtil.java @@ -100,6 +100,9 @@ public static Optional parsePhone(Optional phone) throws IllegalV public static Address parseAddress(String address) throws IllegalValueException { requireNonNull(address); String trimmedAddress = address.trim(); + if (!Address.isValidAddress(trimmedAddress)) { + throw new IllegalValueException(Address.MESSAGE_ADDRESS_CONSTRAINTS); + } return new Address(trimmedAddress); } diff --git a/src/main/java/seedu/address/model/person/Address.java b/src/main/java/seedu/address/model/person/Address.java index a5d467960d22..5e981f07790a 100644 --- a/src/main/java/seedu/address/model/person/Address.java +++ b/src/main/java/seedu/address/model/person/Address.java @@ -1,8 +1,7 @@ package seedu.address.model.person; import static java.util.Objects.requireNonNull; - -import seedu.address.commons.exceptions.IllegalValueException; +import static seedu.address.commons.util.AppUtil.checkArgument; /** * Represents a Person's address in the address book. @@ -22,15 +21,13 @@ public class Address { public final String value; /** - * Validates given address. + * Constructs an {@code Address}. * - * @throws IllegalValueException if given address string is invalid. + * @param address A valid address. */ - public Address(String address) throws IllegalValueException { + public Address(String address) { requireNonNull(address); - if (!isValidAddress(address)) { - throw new IllegalValueException(MESSAGE_ADDRESS_CONSTRAINTS); - } + checkArgument(isValidAddress(address), MESSAGE_ADDRESS_CONSTRAINTS); this.value = address; } diff --git a/src/main/java/seedu/address/storage/XmlAdaptedPerson.java b/src/main/java/seedu/address/storage/XmlAdaptedPerson.java index 48bddccc4b7a..0d2b7d7dc1e7 100644 --- a/src/main/java/seedu/address/storage/XmlAdaptedPerson.java +++ b/src/main/java/seedu/address/storage/XmlAdaptedPerson.java @@ -89,7 +89,12 @@ public Person toModelType() throws IllegalValueException { } final Phone phone = new Phone(this.phone); final Email email = new Email(this.email); + + if (!Address.isValidAddress(this.address)) { + throw new IllegalValueException(Address.MESSAGE_ADDRESS_CONSTRAINTS); + } final Address address = new Address(this.address); + final Set tags = new HashSet<>(personTags); return new Person(name, phone, email, address, tags); } diff --git a/src/test/java/seedu/address/model/person/AddressTest.java b/src/test/java/seedu/address/model/person/AddressTest.java index 4c02e886ddf1..11974544d81d 100644 --- a/src/test/java/seedu/address/model/person/AddressTest.java +++ b/src/test/java/seedu/address/model/person/AddressTest.java @@ -14,6 +14,12 @@ public void constructor_null_throwsNullPointerException() { Assert.assertThrows(NullPointerException.class, () -> new Address(null)); } + @Test + public void constructor_invalidAddress_throwsIllegalArgumentException() { + String invalidAddress = ""; + Assert.assertThrows(IllegalArgumentException.class, () -> new Address(invalidAddress)); + } + @Test public void isValidAddress() { // null address diff --git a/src/test/java/seedu/address/testutil/PersonBuilder.java b/src/test/java/seedu/address/testutil/PersonBuilder.java index fe58a46c2981..a9cecb2e8060 100644 --- a/src/test/java/seedu/address/testutil/PersonBuilder.java +++ b/src/test/java/seedu/address/testutil/PersonBuilder.java @@ -69,11 +69,7 @@ public PersonBuilder withTags(String ... tags) { * Sets the {@code Address} of the {@code Person} that we are building. */ public PersonBuilder withAddress(String address) { - try { - this.person.setAddress(new Address(address)); - } catch (IllegalValueException ive) { - throw new IllegalArgumentException("address is expected to be unique."); - } + this.person.setAddress(new Address(address)); return this; } From 59cf4789e705e487a01fa16bb8c51dc867efeeae Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Tue, 30 May 2017 18:19:07 +0800 Subject: [PATCH 10/20] Move string trimming responsibility from Email to ParserUtil#parseEmail(String) Email's constructor modifies the input string by trimming leading and trailing whitespace. However, given that the model component's sole responsibility is to store data in memory, it shouldn't be modifying any input. Rather, any munging of input should be done in the parser layer. Fix this by moving the string trimming logic from Email to ParserUtil#parseEmail(String). --- .../address/logic/parser/ParserUtil.java | 14 ++++++++++- .../seedu/address/model/person/Email.java | 5 ++-- .../address/logic/parser/ParserUtilTest.java | 23 ++++++++++++------- 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/src/main/java/seedu/address/logic/parser/ParserUtil.java b/src/main/java/seedu/address/logic/parser/ParserUtil.java index f6ac4d033001..511261e033cd 100644 --- a/src/main/java/seedu/address/logic/parser/ParserUtil.java +++ b/src/main/java/seedu/address/logic/parser/ParserUtil.java @@ -115,13 +115,25 @@ public static Optional
parseAddress(Optional address) throws Il return address.isPresent() ? Optional.of(parseAddress(address.get())) : Optional.empty(); } + /** + * Parses a {@code String email} into an {@code Email}. + * Leading and trailing whitespaces will be trimmed. + * + * @throws IllegalValueException if the given {@code email} is invalid. + */ + public static Email parseEmail(String email) throws IllegalValueException { + requireNonNull(email); + String trimmedEmail = email.trim(); + return new Email(trimmedEmail); + } + /** * Parses a {@code Optional email} into an {@code Optional} if {@code email} is present. * See header comment of this class regarding the use of {@code Optional} parameters. */ public static Optional parseEmail(Optional email) throws IllegalValueException { requireNonNull(email); - return email.isPresent() ? Optional.of(new Email(email.get())) : Optional.empty(); + return email.isPresent() ? Optional.of(parseEmail(email.get())) : Optional.empty(); } /** diff --git a/src/main/java/seedu/address/model/person/Email.java b/src/main/java/seedu/address/model/person/Email.java index 514441aa162f..4d2ef6342797 100644 --- a/src/main/java/seedu/address/model/person/Email.java +++ b/src/main/java/seedu/address/model/person/Email.java @@ -23,11 +23,10 @@ public class Email { */ public Email(String email) throws IllegalValueException { requireNonNull(email); - String trimmedEmail = email.trim(); - if (!isValidEmail(trimmedEmail)) { + if (!isValidEmail(email)) { throw new IllegalValueException(MESSAGE_EMAIL_CONSTRAINTS); } - this.value = trimmedEmail; + this.value = email; } /** diff --git a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java index 3f379c75b3ef..9da05d4a20fb 100644 --- a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java +++ b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java @@ -163,15 +163,15 @@ public void parseAddress_validValueWithWhitespace_returnsAddress() throws Except } @Test - public void parseEmail_null_throwsNullPointerException() throws Exception { - thrown.expect(NullPointerException.class); - ParserUtil.parseEmail(null); + public void parseEmail_null_throwsNullPointerException() { + Assert.assertThrows(NullPointerException.class, () -> ParserUtil.parseEmail((String) null)); + Assert.assertThrows(NullPointerException.class, () -> ParserUtil.parseEmail((Optional) null)); } @Test - public void parseEmail_invalidValue_throwsIllegalValueException() throws Exception { - thrown.expect(IllegalValueException.class); - ParserUtil.parseEmail(Optional.of(INVALID_EMAIL)); + public void parseEmail_invalidValue_throwsIllegalValueException() { + Assert.assertThrows(IllegalValueException.class, () -> ParserUtil.parseEmail(INVALID_EMAIL)); + Assert.assertThrows(IllegalValueException.class, () -> ParserUtil.parseEmail(Optional.of(INVALID_EMAIL))); } @Test @@ -182,9 +182,16 @@ public void parseEmail_optionalEmpty_returnsOptionalEmpty() throws Exception { @Test public void parseEmail_validValue_returnsEmail() throws Exception { Email expectedEmail = new Email(VALID_EMAIL); - Optional actualEmail = ParserUtil.parseEmail(Optional.of(VALID_EMAIL)); + assertEquals(expectedEmail, ParserUtil.parseEmail(VALID_EMAIL)); + assertEquals(Optional.of(expectedEmail), ParserUtil.parseEmail(Optional.of(VALID_EMAIL))); + } - assertEquals(expectedEmail, actualEmail.get()); + @Test + public void parseEmail_validValueWithWhitespace_returnsEmail() throws Exception { + String emailWithWhitespace = WHITESPACE + VALID_EMAIL + WHITESPACE; + Email expectedEmail = new Email(VALID_EMAIL); + assertEquals(expectedEmail, ParserUtil.parseEmail(emailWithWhitespace)); + assertEquals(Optional.of(expectedEmail), ParserUtil.parseEmail(Optional.of(emailWithWhitespace))); } @Test From 8c73aedfb82b685889b106aa0f0a98e5f66dd7d1 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Tue, 30 May 2017 18:37:00 +0800 Subject: [PATCH 11/20] Move Email validation step from model to parser and storage layers The Email(String) constructor validates the passed in email String, throwing a checked IllegalValueException error if the input fails validation. However, according to our architecture, the model layer should only be concerned with the storage of data in memory. Any data which reaches the model layer should have already been validated. As such, if for whatever reason the input email String fails validation in the model layer, it is most likely a programmer error, and a runtime exception should thus be thrown instead. Fix this by moving the input validation step, and the throwing of the checked IllegalValueException, from the Email constructor to the parser and storage layers. The Email constructor will instead use the checkArgument(...) helper function to verify the "valid email format" precondition. Thanks to this fix, as Email's constructor does not throw checked exceptions any more, we can remove a try/catch block from PersonBuilder as well (otherwise the code won't compile). --- .../java/seedu/address/logic/parser/ParserUtil.java | 3 +++ src/main/java/seedu/address/model/person/Email.java | 13 +++++-------- .../seedu/address/storage/XmlAdaptedPerson.java | 4 ++++ .../java/seedu/address/model/person/EmailTest.java | 6 ++++++ .../java/seedu/address/testutil/PersonBuilder.java | 6 +----- 5 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/main/java/seedu/address/logic/parser/ParserUtil.java b/src/main/java/seedu/address/logic/parser/ParserUtil.java index 511261e033cd..166e6a974c24 100644 --- a/src/main/java/seedu/address/logic/parser/ParserUtil.java +++ b/src/main/java/seedu/address/logic/parser/ParserUtil.java @@ -124,6 +124,9 @@ public static Optional
parseAddress(Optional address) throws Il public static Email parseEmail(String email) throws IllegalValueException { requireNonNull(email); String trimmedEmail = email.trim(); + if (!Email.isValidEmail(trimmedEmail)) { + throw new IllegalValueException(Email.MESSAGE_EMAIL_CONSTRAINTS); + } return new Email(trimmedEmail); } diff --git a/src/main/java/seedu/address/model/person/Email.java b/src/main/java/seedu/address/model/person/Email.java index 4d2ef6342797..d636088abb63 100644 --- a/src/main/java/seedu/address/model/person/Email.java +++ b/src/main/java/seedu/address/model/person/Email.java @@ -1,8 +1,7 @@ package seedu.address.model.person; import static java.util.Objects.requireNonNull; - -import seedu.address.commons.exceptions.IllegalValueException; +import static seedu.address.commons.util.AppUtil.checkArgument; /** * Represents a Person's phone number in the address book. @@ -17,15 +16,13 @@ public class Email { public final String value; /** - * Validates given email. + * Constructs an {@code Email}. * - * @throws IllegalValueException if given email address string is invalid. + * @param email A valid email address. */ - public Email(String email) throws IllegalValueException { + public Email(String email) { requireNonNull(email); - if (!isValidEmail(email)) { - throw new IllegalValueException(MESSAGE_EMAIL_CONSTRAINTS); - } + checkArgument(isValidEmail(email), MESSAGE_EMAIL_CONSTRAINTS); this.value = email; } diff --git a/src/main/java/seedu/address/storage/XmlAdaptedPerson.java b/src/main/java/seedu/address/storage/XmlAdaptedPerson.java index 0d2b7d7dc1e7..5e833226e2b1 100644 --- a/src/main/java/seedu/address/storage/XmlAdaptedPerson.java +++ b/src/main/java/seedu/address/storage/XmlAdaptedPerson.java @@ -88,6 +88,10 @@ public Person toModelType() throws IllegalValueException { throw new IllegalValueException(Phone.MESSAGE_PHONE_CONSTRAINTS); } final Phone phone = new Phone(this.phone); + + if (!Email.isValidEmail(this.email)) { + throw new IllegalValueException(Email.MESSAGE_EMAIL_CONSTRAINTS); + } final Email email = new Email(this.email); if (!Address.isValidAddress(this.address)) { diff --git a/src/test/java/seedu/address/model/person/EmailTest.java b/src/test/java/seedu/address/model/person/EmailTest.java index 1d62e1b5772a..c1ba86c4d61f 100644 --- a/src/test/java/seedu/address/model/person/EmailTest.java +++ b/src/test/java/seedu/address/model/person/EmailTest.java @@ -14,6 +14,12 @@ public void constructor_null_throwsNullPointerException() { Assert.assertThrows(NullPointerException.class, () -> new Email(null)); } + @Test + public void constructor_invalidEmail_throwsIllegalArgumentException() { + String invalidEmail = ""; + Assert.assertThrows(IllegalArgumentException.class, () -> new Email(invalidEmail)); + } + @Test public void isValidEmail() { // null email diff --git a/src/test/java/seedu/address/testutil/PersonBuilder.java b/src/test/java/seedu/address/testutil/PersonBuilder.java index a9cecb2e8060..fe55a33702f7 100644 --- a/src/test/java/seedu/address/testutil/PersonBuilder.java +++ b/src/test/java/seedu/address/testutil/PersonBuilder.java @@ -85,11 +85,7 @@ public PersonBuilder withPhone(String phone) { * Sets the {@code Email} of the {@code Person} that we are building. */ public PersonBuilder withEmail(String email) { - try { - this.person.setEmail(new Email(email)); - } catch (IllegalValueException ive) { - throw new IllegalArgumentException("email is expected to be unique."); - } + this.person.setEmail(new Email(email)); return this; } From 702e28ab6cd7a88e93e416fe811b4c18a583ecb4 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Tue, 30 May 2017 18:24:26 +0800 Subject: [PATCH 12/20] Move string trimming responsibility from Tag to ParserUtil#parseTag(String) Tag's constructor modifies the input string by trimming leading and trailing whitespace. However, given that the model component's sole responsibility is to store data in memory, it shouldn't be modifying any input. Rather, any munging of input should be done in the parser layer. Fix this by moving the string trimming logic from Tag to ParserUtil#parseTag(String). --- .../address/logic/parser/ParserUtil.java | 14 ++++++++++- .../java/seedu/address/model/tag/Tag.java | 5 ++-- .../address/logic/parser/ParserUtilTest.java | 25 +++++++++++++++++++ 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/src/main/java/seedu/address/logic/parser/ParserUtil.java b/src/main/java/seedu/address/logic/parser/ParserUtil.java index 166e6a974c24..e7a6c403508b 100644 --- a/src/main/java/seedu/address/logic/parser/ParserUtil.java +++ b/src/main/java/seedu/address/logic/parser/ParserUtil.java @@ -139,6 +139,18 @@ public static Optional parseEmail(Optional email) throws IllegalV return email.isPresent() ? Optional.of(parseEmail(email.get())) : Optional.empty(); } + /** + * Parses a {@code String tag} into a {@code Tag}. + * Leading and trailing whitespaces will be trimmed. + * + * @throws IllegalValueException if the given {@code tag} is invalid. + */ + public static Tag parseTag(String tag) throws IllegalValueException { + requireNonNull(tag); + String trimmedTag = tag.trim(); + return new Tag(trimmedTag); + } + /** * Parses {@code Collection tags} into a {@code Set}. */ @@ -146,7 +158,7 @@ public static Set parseTags(Collection tags) throws IllegalValueExc requireNonNull(tags); final Set tagSet = new HashSet<>(); for (String tagName : tags) { - tagSet.add(new Tag(tagName)); + tagSet.add(parseTag(tagName)); } return tagSet; } diff --git a/src/main/java/seedu/address/model/tag/Tag.java b/src/main/java/seedu/address/model/tag/Tag.java index c217550c7e8f..b5cc134b4da0 100644 --- a/src/main/java/seedu/address/model/tag/Tag.java +++ b/src/main/java/seedu/address/model/tag/Tag.java @@ -22,11 +22,10 @@ public class Tag { */ public Tag(String name) throws IllegalValueException { requireNonNull(name); - String trimmedName = name.trim(); - if (!isValidTagName(trimmedName)) { + if (!isValidTagName(name)) { throw new IllegalValueException(MESSAGE_TAG_CONSTRAINTS); } - this.tagName = trimmedName; + this.tagName = name; } /** diff --git a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java index 9da05d4a20fb..d856c71796e7 100644 --- a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java +++ b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java @@ -194,6 +194,31 @@ public void parseEmail_validValueWithWhitespace_returnsEmail() throws Exception assertEquals(Optional.of(expectedEmail), ParserUtil.parseEmail(Optional.of(emailWithWhitespace))); } + @Test + public void parseTag_null_throwsNullPointerException() throws Exception { + thrown.expect(NullPointerException.class); + ParserUtil.parseTag(null); + } + + @Test + public void parseTag_invalidValue_throwsIllegalValueException() throws Exception { + thrown.expect(IllegalValueException.class); + ParserUtil.parseTag(INVALID_TAG); + } + + @Test + public void parseTag_validValue_returnsTag() throws Exception { + Tag expectedTag = new Tag(VALID_TAG_1); + assertEquals(expectedTag, ParserUtil.parseTag(VALID_TAG_1)); + } + + @Test + public void parseTag_validValueWithWhitespace_returnsTag() throws Exception { + String tagWithWhitespace = WHITESPACE + VALID_TAG_1 + WHITESPACE; + Tag expectedTag = new Tag(VALID_TAG_1); + assertEquals(expectedTag, ParserUtil.parseTag(tagWithWhitespace)); + } + @Test public void parseTags_null_throwsNullPointerException() throws Exception { thrown.expect(NullPointerException.class); From eb45188b26954e5dddedcfcd210cb38631f41576 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Tue, 30 May 2017 18:40:47 +0800 Subject: [PATCH 13/20] Move Tag validation step from model to parser and storage layers The Tag(String) constructor validates the passed in tag String, throwing a checked IllegalValueException error if the input fails validation. However, according to our architecture, the model layer should only be concerned with the storage of data in memory. Any data which reaches the model layer should have already been validated. As such, if for any reason the input tag String fails validation in the model layer, it is most likely a programmer error, and a runtime exception should thus be thrown instead. Fix this by moving the input validation step, and the throwing of the checked IllegalValueException, from the Tag constructor to the parser and storage layers. The Tag constructor will instead use the checkArgument(...) helper function to verify the "valid tag format" precondition. While we are here, rename the argument `name` to the more descriptive `tagName`. --- .../seedu/address/logic/parser/ParserUtil.java | 3 +++ src/main/java/seedu/address/model/tag/Tag.java | 17 +++++++---------- .../seedu/address/storage/XmlAdaptedTag.java | 3 +++ .../java/seedu/address/model/tag/TagTest.java | 6 ++++++ 4 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/main/java/seedu/address/logic/parser/ParserUtil.java b/src/main/java/seedu/address/logic/parser/ParserUtil.java index e7a6c403508b..5d6d4ae3f7b1 100644 --- a/src/main/java/seedu/address/logic/parser/ParserUtil.java +++ b/src/main/java/seedu/address/logic/parser/ParserUtil.java @@ -148,6 +148,9 @@ public static Optional parseEmail(Optional email) throws IllegalV public static Tag parseTag(String tag) throws IllegalValueException { requireNonNull(tag); String trimmedTag = tag.trim(); + if (!Tag.isValidTagName(trimmedTag)) { + throw new IllegalValueException(Tag.MESSAGE_TAG_CONSTRAINTS); + } return new Tag(trimmedTag); } diff --git a/src/main/java/seedu/address/model/tag/Tag.java b/src/main/java/seedu/address/model/tag/Tag.java index b5cc134b4da0..65bdd769995d 100644 --- a/src/main/java/seedu/address/model/tag/Tag.java +++ b/src/main/java/seedu/address/model/tag/Tag.java @@ -1,8 +1,7 @@ package seedu.address.model.tag; import static java.util.Objects.requireNonNull; - -import seedu.address.commons.exceptions.IllegalValueException; +import static seedu.address.commons.util.AppUtil.checkArgument; /** * Represents a Tag in the address book. @@ -16,16 +15,14 @@ public class Tag { public final String tagName; /** - * Validates given tag name. + * Constructs a {@code Tag}. * - * @throws IllegalValueException if the given tag name string is invalid. + * @param tagName A valid tag name. */ - public Tag(String name) throws IllegalValueException { - requireNonNull(name); - if (!isValidTagName(name)) { - throw new IllegalValueException(MESSAGE_TAG_CONSTRAINTS); - } - this.tagName = name; + public Tag(String tagName) { + requireNonNull(tagName); + checkArgument(isValidTagName(tagName), MESSAGE_TAG_CONSTRAINTS); + this.tagName = tagName; } /** diff --git a/src/main/java/seedu/address/storage/XmlAdaptedTag.java b/src/main/java/seedu/address/storage/XmlAdaptedTag.java index f7775fcd98f2..4e9d3648f556 100644 --- a/src/main/java/seedu/address/storage/XmlAdaptedTag.java +++ b/src/main/java/seedu/address/storage/XmlAdaptedTag.java @@ -41,6 +41,9 @@ public XmlAdaptedTag(Tag source) { * @throws IllegalValueException if there were any data constraints violated in the adapted person */ public Tag toModelType() throws IllegalValueException { + if (!Tag.isValidTagName(tagName)) { + throw new IllegalValueException(Tag.MESSAGE_TAG_CONSTRAINTS); + } return new Tag(tagName); } diff --git a/src/test/java/seedu/address/model/tag/TagTest.java b/src/test/java/seedu/address/model/tag/TagTest.java index 38c5a7be1a40..80bfd156d816 100644 --- a/src/test/java/seedu/address/model/tag/TagTest.java +++ b/src/test/java/seedu/address/model/tag/TagTest.java @@ -11,6 +11,12 @@ public void constructor_null_throwsNullPointerException() { Assert.assertThrows(NullPointerException.class, () -> new Tag(null)); } + @Test + public void constructor_invalidTagName_throwsIllegalArgumentException() { + String invalidTagName = ""; + Assert.assertThrows(IllegalArgumentException.class, () -> new Tag(invalidTagName)); + } + @Test public void isValidTagName() { // null tag name From e73602a424dff981282769d244f4f28267daeff8 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Tue, 30 May 2017 18:45:56 +0800 Subject: [PATCH 14/20] Remove unnecessary IllegalValueException catches and throws in code base In the previous commits, we have fixed up the Name, Email, Address and Tag classes to not throw the checked IllegalValueException any more. As a result of these changes, there are many other call sites in the code base which do not need to throw or catch IllegalValueException any more. Let's clean up these call sites. --- .../address/model/util/SampleDataUtil.java | 47 +++++++++---------- .../seedu/address/testutil/PersonBuilder.java | 23 +++------ 2 files changed, 28 insertions(+), 42 deletions(-) diff --git a/src/main/java/seedu/address/model/util/SampleDataUtil.java b/src/main/java/seedu/address/model/util/SampleDataUtil.java index 02c4204199df..aea96bfb31f3 100644 --- a/src/main/java/seedu/address/model/util/SampleDataUtil.java +++ b/src/main/java/seedu/address/model/util/SampleDataUtil.java @@ -3,7 +3,6 @@ import java.util.HashSet; import java.util.Set; -import seedu.address.commons.exceptions.IllegalValueException; import seedu.address.model.AddressBook; import seedu.address.model.ReadOnlyAddressBook; import seedu.address.model.person.Address; @@ -19,30 +18,26 @@ */ public class SampleDataUtil { public static Person[] getSamplePersons() { - try { - return new Person[] { - new Person(new Name("Alex Yeoh"), new Phone("87438807"), new Email("alexyeoh@example.com"), - new Address("Blk 30 Geylang Street 29, #06-40"), - getTagSet("friends")), - new Person(new Name("Bernice Yu"), new Phone("99272758"), new Email("berniceyu@example.com"), - new Address("Blk 30 Lorong 3 Serangoon Gardens, #07-18"), - getTagSet("colleagues", "friends")), - new Person(new Name("Charlotte Oliveiro"), new Phone("93210283"), new Email("charlotte@example.com"), - new Address("Blk 11 Ang Mo Kio Street 74, #11-04"), - getTagSet("neighbours")), - new Person(new Name("David Li"), new Phone("91031282"), new Email("lidavid@example.com"), - new Address("Blk 436 Serangoon Gardens Street 26, #16-43"), - getTagSet("family")), - new Person(new Name("Irfan Ibrahim"), new Phone("92492021"), new Email("irfan@example.com"), - new Address("Blk 47 Tampines Street 20, #17-35"), - getTagSet("classmates")), - new Person(new Name("Roy Balakrishnan"), new Phone("92624417"), new Email("royb@example.com"), - new Address("Blk 45 Aljunied Street 85, #11-31"), - getTagSet("colleagues")) - }; - } catch (IllegalValueException e) { - throw new AssertionError("sample data cannot be invalid", e); - } + return new Person[] { + new Person(new Name("Alex Yeoh"), new Phone("87438807"), new Email("alexyeoh@example.com"), + new Address("Blk 30 Geylang Street 29, #06-40"), + getTagSet("friends")), + new Person(new Name("Bernice Yu"), new Phone("99272758"), new Email("berniceyu@example.com"), + new Address("Blk 30 Lorong 3 Serangoon Gardens, #07-18"), + getTagSet("colleagues", "friends")), + new Person(new Name("Charlotte Oliveiro"), new Phone("93210283"), new Email("charlotte@example.com"), + new Address("Blk 11 Ang Mo Kio Street 74, #11-04"), + getTagSet("neighbours")), + new Person(new Name("David Li"), new Phone("91031282"), new Email("lidavid@example.com"), + new Address("Blk 436 Serangoon Gardens Street 26, #16-43"), + getTagSet("family")), + new Person(new Name("Irfan Ibrahim"), new Phone("92492021"), new Email("irfan@example.com"), + new Address("Blk 47 Tampines Street 20, #17-35"), + getTagSet("classmates")), + new Person(new Name("Roy Balakrishnan"), new Phone("92624417"), new Email("royb@example.com"), + new Address("Blk 45 Aljunied Street 85, #11-31"), + getTagSet("colleagues")) + }; } public static ReadOnlyAddressBook getSampleAddressBook() { @@ -60,7 +55,7 @@ public static ReadOnlyAddressBook getSampleAddressBook() { /** * Returns a tag set containing the list of strings given. */ - public static Set getTagSet(String... strings) throws IllegalValueException { + public static Set getTagSet(String... strings) { HashSet tags = new HashSet<>(); for (String s : strings) { tags.add(new Tag(s)); diff --git a/src/test/java/seedu/address/testutil/PersonBuilder.java b/src/test/java/seedu/address/testutil/PersonBuilder.java index fe55a33702f7..4a2095046b88 100644 --- a/src/test/java/seedu/address/testutil/PersonBuilder.java +++ b/src/test/java/seedu/address/testutil/PersonBuilder.java @@ -2,7 +2,6 @@ import java.util.Set; -import seedu.address.commons.exceptions.IllegalValueException; import seedu.address.model.person.Address; import seedu.address.model.person.Email; import seedu.address.model.person.Name; @@ -26,16 +25,12 @@ public class PersonBuilder { private Person person; public PersonBuilder() { - try { - Name defaultName = new Name(DEFAULT_NAME); - Phone defaultPhone = new Phone(DEFAULT_PHONE); - Email defaultEmail = new Email(DEFAULT_EMAIL); - Address defaultAddress = new Address(DEFAULT_ADDRESS); - Set defaultTags = SampleDataUtil.getTagSet(DEFAULT_TAGS); - this.person = new Person(defaultName, defaultPhone, defaultEmail, defaultAddress, defaultTags); - } catch (IllegalValueException ive) { - throw new AssertionError("Default person's values are invalid."); - } + Name defaultName = new Name(DEFAULT_NAME); + Phone defaultPhone = new Phone(DEFAULT_PHONE); + Email defaultEmail = new Email(DEFAULT_EMAIL); + Address defaultAddress = new Address(DEFAULT_ADDRESS); + Set defaultTags = SampleDataUtil.getTagSet(DEFAULT_TAGS); + this.person = new Person(defaultName, defaultPhone, defaultEmail, defaultAddress, defaultTags); } /** @@ -57,11 +52,7 @@ public PersonBuilder withName(String name) { * Parses the {@code tags} into a {@code Set} and set it to the {@code Person} that we are building. */ public PersonBuilder withTags(String ... tags) { - try { - this.person.setTags(SampleDataUtil.getTagSet(tags)); - } catch (IllegalValueException ive) { - throw new IllegalArgumentException("tags are expected to be unique."); - } + this.person.setTags(SampleDataUtil.getTagSet(tags)); return this; } From 24119fd1aa67376e0d0276d1e15439b82f68626f Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Sun, 10 Dec 2017 00:47:22 +0800 Subject: [PATCH 15/20] fixup! XmlAdaptedPersonTest: add tests for #toModelType() --- .../address/storage/XmlAdaptedPersonTest.java | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/test/java/seedu/address/storage/XmlAdaptedPersonTest.java b/src/test/java/seedu/address/storage/XmlAdaptedPersonTest.java index e38b88839884..2b4b0af30960 100644 --- a/src/test/java/seedu/address/storage/XmlAdaptedPersonTest.java +++ b/src/test/java/seedu/address/storage/XmlAdaptedPersonTest.java @@ -24,8 +24,8 @@ public class XmlAdaptedPersonTest { private static final String VALID_EMAIL = BENSON.getEmail().toString(); private static final String VALID_ADDRESS = BENSON.getAddress().toString(); - private final List validTags = BENSON.getTags().stream() - .map(tag -> new XmlAdaptedTag(tag)) + private static final List VALID_TAGS = BENSON.getTags().stream() + .map(XmlAdaptedTag::new) .collect(Collectors.toList()); @Test @@ -37,38 +37,38 @@ public void toModelType_validPersonDetails_returnsPerson() throws Exception { @Test public void toModelType_invalidName_throwsIllegalValueException() { XmlAdaptedPerson person = - new XmlAdaptedPerson(INVALID_NAME, VALID_PHONE, VALID_EMAIL, VALID_ADDRESS, validTags); - Assert.assertThrows(IllegalValueException.class, () -> person.toModelType()); + new XmlAdaptedPerson(INVALID_NAME, VALID_PHONE, VALID_EMAIL, VALID_ADDRESS, VALID_TAGS); + Assert.assertThrows(IllegalValueException.class, person::toModelType); } @Test public void toModelType_invalidPhone_throwsIllegalValueException() { XmlAdaptedPerson person = - new XmlAdaptedPerson(VALID_NAME, INVALID_PHONE, VALID_EMAIL, VALID_ADDRESS, validTags); - Assert.assertThrows(IllegalValueException.class, () -> person.toModelType()); + new XmlAdaptedPerson(VALID_NAME, INVALID_PHONE, VALID_EMAIL, VALID_ADDRESS, VALID_TAGS); + Assert.assertThrows(IllegalValueException.class, person::toModelType); } @Test public void toModelType_invalidEmail_throwsIllegalValueException() { XmlAdaptedPerson person = - new XmlAdaptedPerson(VALID_NAME, VALID_PHONE, INVALID_EMAIL, VALID_ADDRESS, validTags); - Assert.assertThrows(IllegalValueException.class, () -> person.toModelType()); + new XmlAdaptedPerson(VALID_NAME, VALID_PHONE, INVALID_EMAIL, VALID_ADDRESS, VALID_TAGS); + Assert.assertThrows(IllegalValueException.class, person::toModelType); } @Test public void toModelType_invalidAddress_throwsIllegalValueException() { XmlAdaptedPerson person = - new XmlAdaptedPerson(VALID_NAME, VALID_PHONE, VALID_EMAIL, INVALID_ADDRESS, validTags); - Assert.assertThrows(IllegalValueException.class, () -> person.toModelType()); + new XmlAdaptedPerson(VALID_NAME, VALID_PHONE, VALID_EMAIL, INVALID_ADDRESS, VALID_TAGS); + Assert.assertThrows(IllegalValueException.class, person::toModelType); } @Test public void toModelType_invalidTags_throwsIllegalValueException() { - List invalidTags = new ArrayList<>(validTags); + List invalidTags = new ArrayList<>(VALID_TAGS); invalidTags.add(new XmlAdaptedTag(INVALID_TAG)); XmlAdaptedPerson person = new XmlAdaptedPerson(VALID_NAME, VALID_PHONE, VALID_EMAIL, VALID_ADDRESS, invalidTags); - Assert.assertThrows(IllegalValueException.class, () -> person.toModelType()); + Assert.assertThrows(IllegalValueException.class, person::toModelType); } } From 7760457c1925e089b0904c939a191ff2cb5d86cf Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Sun, 10 Dec 2017 00:51:49 +0800 Subject: [PATCH 16/20] fixup! Move string trimming responsibility from Name to ParserUtil#parseName(String) --- src/test/java/seedu/address/logic/parser/ParserUtilTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java index d856c71796e7..7cc8613fa717 100644 --- a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java +++ b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java @@ -84,7 +84,7 @@ public void parseName_optionalEmpty_returnsOptionalEmpty() throws Exception { } @Test - public void parseName_validValue_returnsName() throws Exception { + public void parseName_validValueWithoutWhitespace_returnsName() throws Exception { Name expectedName = new Name(VALID_NAME); assertEquals(expectedName, ParserUtil.parseName(VALID_NAME)); assertEquals(Optional.of(expectedName), ParserUtil.parseName(Optional.of(VALID_NAME))); From 1a71e63ec4cc8c1515e285f741c5b95199855cfc Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Sun, 10 Dec 2017 00:52:00 +0800 Subject: [PATCH 17/20] fixup! Move string trimming responsibility from Phone to ParserUtil#parsePhone(String) --- src/test/java/seedu/address/logic/parser/ParserUtilTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java index 7cc8613fa717..4c581b27f77f 100644 --- a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java +++ b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java @@ -116,14 +116,14 @@ public void parsePhone_optionalEmpty_returnsOptionalEmpty() throws Exception { } @Test - public void parsePhone_validValue_returnsPhone() throws Exception { + public void parsePhone_validValueWithoutWhitespace_returnsPhone() throws Exception { Phone expectedPhone = new Phone(VALID_PHONE); assertEquals(expectedPhone, ParserUtil.parsePhone(VALID_PHONE)); assertEquals(Optional.of(expectedPhone), ParserUtil.parsePhone(Optional.of(VALID_PHONE))); } @Test - public void parsePhone_validValueWithWhitespace_returnsPhone() throws Exception { + public void parsePhone_validValueWithWhitespace_returnsTrimmedPhone() throws Exception { String phoneWithWhitespace = WHITESPACE + VALID_PHONE + WHITESPACE; Phone expectedPhone = new Phone(VALID_PHONE); assertEquals(expectedPhone, ParserUtil.parsePhone(phoneWithWhitespace)); From c008d9b595aa393e39bf8d177a60b15a0cd88f8c Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Sun, 10 Dec 2017 00:52:09 +0800 Subject: [PATCH 18/20] fixup! Move string trimming responsibility from Address to ParserUtil#parseAddress(String) --- src/test/java/seedu/address/logic/parser/ParserUtilTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java index 4c581b27f77f..9dc42677a9a3 100644 --- a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java +++ b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java @@ -148,14 +148,14 @@ public void parseAddress_optionalEmpty_returnsOptionalEmpty() throws Exception { } @Test - public void parseAddress_validValue_returnsAddress() throws Exception { + public void parseAddress_validValueWithoutWhitespace_returnsAddress() throws Exception { Address expectedAddress = new Address(VALID_ADDRESS); assertEquals(expectedAddress, ParserUtil.parseAddress(VALID_ADDRESS)); assertEquals(Optional.of(expectedAddress), ParserUtil.parseAddress(Optional.of(VALID_ADDRESS))); } @Test - public void parseAddress_validValueWithWhitespace_returnsAddress() throws Exception { + public void parseAddress_validValueWithWhitespace_returnsTrimmedAddress() throws Exception { String addressWithWhitespace = WHITESPACE + VALID_ADDRESS + WHITESPACE; Address expectedAddress = new Address(VALID_ADDRESS); assertEquals(expectedAddress, ParserUtil.parseAddress(addressWithWhitespace)); From 44c56a3223d4f748fa31a75341d1f85c2028592d Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Sun, 10 Dec 2017 00:52:17 +0800 Subject: [PATCH 19/20] fixup! Move string trimming responsibility from Email to ParserUtil#parseEmail(String) --- src/test/java/seedu/address/logic/parser/ParserUtilTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java index 9dc42677a9a3..ac53cb34f019 100644 --- a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java +++ b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java @@ -180,14 +180,14 @@ public void parseEmail_optionalEmpty_returnsOptionalEmpty() throws Exception { } @Test - public void parseEmail_validValue_returnsEmail() throws Exception { + public void parseEmail_validValueWithoutWhitespace_returnsEmail() throws Exception { Email expectedEmail = new Email(VALID_EMAIL); assertEquals(expectedEmail, ParserUtil.parseEmail(VALID_EMAIL)); assertEquals(Optional.of(expectedEmail), ParserUtil.parseEmail(Optional.of(VALID_EMAIL))); } @Test - public void parseEmail_validValueWithWhitespace_returnsEmail() throws Exception { + public void parseEmail_validValueWithWhitespace_returnsTrimmedEmail() throws Exception { String emailWithWhitespace = WHITESPACE + VALID_EMAIL + WHITESPACE; Email expectedEmail = new Email(VALID_EMAIL); assertEquals(expectedEmail, ParserUtil.parseEmail(emailWithWhitespace)); From 0ccf7ec582542542de59652f30e5c39caed05224 Mon Sep 17 00:00:00 2001 From: Paul Tan Date: Sun, 10 Dec 2017 00:52:27 +0800 Subject: [PATCH 20/20] fixup! Move string trimming responsibility from Tag to ParserUtil#parseTag(String) --- src/test/java/seedu/address/logic/parser/ParserUtilTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java index ac53cb34f019..54516c1c5e95 100644 --- a/src/test/java/seedu/address/logic/parser/ParserUtilTest.java +++ b/src/test/java/seedu/address/logic/parser/ParserUtilTest.java @@ -207,13 +207,13 @@ public void parseTag_invalidValue_throwsIllegalValueException() throws Exception } @Test - public void parseTag_validValue_returnsTag() throws Exception { + public void parseTag_validValueWithoutWhitespace_returnsTag() throws Exception { Tag expectedTag = new Tag(VALID_TAG_1); assertEquals(expectedTag, ParserUtil.parseTag(VALID_TAG_1)); } @Test - public void parseTag_validValueWithWhitespace_returnsTag() throws Exception { + public void parseTag_validValueWithWhitespace_returnsTrimmedTag() throws Exception { String tagWithWhitespace = WHITESPACE + VALID_TAG_1 + WHITESPACE; Tag expectedTag = new Tag(VALID_TAG_1); assertEquals(expectedTag, ParserUtil.parseTag(tagWithWhitespace));