[REFACTOR] Add and remove not useful person's fields #18

WilliamRyank · 2019-09-29T14:03:28Z

No description provided.

C-likethis123 · 2019-09-29T14:33:07Z

LICENSE

@@ -6,7 +6,7 @@ Permission is hereby granted, free of charge, to any person obtaining a copy
 of this software and associated documentation files (the "Software"), to deal


Just FYI,

'persons' is a term that can be used in legal writing. This license is legal writing, so it's okay to put it as 'persons'.

Don't change licenses or copyright notices.

i mass changed the word person i think so it changes the license too ill added later

Noted on this.

C-likethis123 · 2019-09-29T14:34:58Z

docs/DeveloperGuide.adoc

@@ -320,8 +320,8 @@ _{More to be added}_

 *MSS*

-1.  User requests to list persons
-2.  AddressBook shows a list of persons
+1.  User requests to list people


It seems that the developer guide is about the Address Book, not our software. Could you update the contents of the developer's guide to be specific to our software?

C-likethis123 · 2019-09-29T14:35:41Z

docs/UserGuide.adoc

@@ -34,7 +34,7 @@ e.g. typing *`help`* and pressing kbd:[Enter] will open the help window.
 .  Some example commands you can try:

 * *`list`* : lists all contacts
-* **`add`**`n/John Doe p/98765432 e/johnd@example.com a/John street, block 123, #01-01` : adds a contact named `John Doe` to the Address Book.


While I appreciate the work done, there is no need for you to edit the User Guide as I have drafted a User Guide and made a PR.

C-likethis123 · 2019-09-29T14:42:35Z

src/main/java/seedu/address/logic/commands/EditCommand.java

@@ -87,19 +80,18 @@ public CommandResult execute(Model model) throws CommandException {
    }

    /**
-     * Creates and returns a {@code Person} with the details of {@code personToEdit}
+     * Creates and returns a {@code person} with the details of {@code personToEdit}


Instead of {@code person}, it should be {@code Person} because 'Person' is the class name.

C-likethis123 · 2019-09-29T14:45:50Z

src/main/java/seedu/address/logic/parser/CliSyntax.java

@@ -7,9 +7,7 @@

    /* Prefix definitions */
    public static final Prefix PREFIX_NAME = new Prefix("n/");
+    public static final Prefix PREFIX_NRIC = new Prefix("ic/");
    public static final Prefix PREFIX_PHONE = new Prefix("p/");


Just a point for discussion, should our prefix be '/c' for Contact, or '/p' for Phone?

C-likethis123 · 2019-09-29T14:54:23Z

src/main/java/seedu/address/model/AddressBook.java

@@ -77,7 +77,8 @@ public void addPerson(Person p) {
    /**
     * Replaces the given person {@code target} in the list with {@code editedPerson}.
     * {@code target} must exist in the address book.
-     * The person identity of {@code editedPerson} must not be the same as another existing person in the address book.
+     * The person identity of {@code editedPerson} must not be the same as another existing person


While this comment is informative, perhaps a better way to word it would be 'The identity of {@code editedPerson} should not be the same as another existing {@code Person}'?
Also, the context needs to be changed, because we are changing this from the Address Book to the ORGANice Transplant Manager

C-likethis123 · 2019-09-29T15:03:18Z

src/main/java/seedu/address/model/person/Nric.java


-    public static final String MESSAGE_CONSTRAINTS = "Addresses can take any values, and it should not be blank";
+    public static final String MESSAGE_CONSTRAINTS = "Nric must be in the form [S/T/F/G][7-numbers][A-Z], "
+            + "and it should not be blank";

    /*


Perhaps a better comment would explain the regex on what makes a valid NRIC, such as:

It must start with S, T, F or G

It must have 7 digits afterward

it must end with a letter

C-likethis123 · 2019-09-29T15:10:36Z

src/main/java/seedu/address/model/person/Phone.java

@@ -4,7 +4,7 @@
 import static seedu.address.commons.util.AppUtil.checkArgument;

 /**
- * Represents a Person's phone number in the address book.
+ * Represents a person's phone number in the address book.


I think this comment should remain as 'Represent a Person's...' instead of 'person'. The reason is that 'Person' represents the class, while 'person' is an English term. This comment is meant to be a technical description of what this class is. In this case, this class represents the phone number of Person objects. Therefore, I think 'Person' is a more apt choice.

C-likethis123 · 2019-09-29T15:13:16Z

src/main/java/seedu/address/storage/JsonAdaptedPerson.java

    }

    /**
-     * Converts this Jackson-friendly adapted person object into the model's {@code Person} object.
+     * Converts this Jackson-friendly adapted person object into the model's {@code person} object.


Same issue, 'person' vs 'Person'

C-likethis123 · 2019-09-29T15:14:01Z

src/main/java/seedu/address/ui/PersonCard.java

 import javafx.scene.layout.HBox;
 import javafx.scene.layout.Region;
 import seedu.address.model.person.Person;

 /**
- * An UI component that displays information of a {@code Person}.
+ * An UI component that displays information of a {@code person}.


Same issue as the 'person'/'Person' issue

C-likethis123 · 2019-09-29T15:14:22Z

src/main/java/seedu/address/ui/PersonListPanel.java

@@ -27,7 +27,7 @@ public PersonListPanel(ObservableList<Person> personList) {
    }

    /**
-     * Custom {@code ListCell} that displays the graphics of a {@code Person} using a {@code PersonCard}.
+     * Custom {@code ListCell} that displays the graphics of a {@code person} using a {@code PersonCard}.


Same issue as the 'person'/'Person' issue

…nk/main into refactor-person-class

iskandarzulkarnaien · 2019-10-01T08:27:50Z

This PR may have an issue with Travis. If need to merge soon, see if creating another PR will work. See #26

WilliamRyank and others added 6 commits September 25, 2019 16:39

Remove email field

dbe996f

fix checkstyle error

f546bab

Remove tags field

e2d4890

Add nric field to person

1549501

Add person type field

2e9f40c

Merge branch 'master' into refactor-person-class

e0ef2fc

C-likethis123 reviewed Sep 29, 2019

View reviewed changes

WilliamRyank added 2 commits September 30, 2019 23:14

Fixing some style

0ef4fe7

Merge branch 'refactor-person-class' of https://github.com/WilliamRya…

681053b

…nk/main into refactor-person-class

iskandarzulkarnaien mentioned this pull request Oct 1, 2019

Travis may not be working #26

Closed

WilliamRyank closed this Oct 1, 2019

nus-se-bot mentioned this pull request Oct 3, 2019

Feedback on week 7 team project progress for CS2103T-T13-1 #36

Open

34 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REFACTOR] Add and remove not useful person's fields #18

[REFACTOR] Add and remove not useful person's fields #18

WilliamRyank commented Sep 29, 2019

C-likethis123 Sep 29, 2019

WilliamRyank Sep 29, 2019

C-likethis123 Sep 29, 2019

C-likethis123 Sep 29, 2019

C-likethis123 Sep 29, 2019

C-likethis123 Sep 29, 2019

C-likethis123 Sep 29, 2019

C-likethis123 Sep 29, 2019

C-likethis123 Sep 29, 2019

C-likethis123 Sep 29, 2019

C-likethis123 Sep 29, 2019

C-likethis123 Sep 29, 2019

C-likethis123 Sep 29, 2019

iskandarzulkarnaien commented Oct 1, 2019

		@@ -6,7 +6,7 @@ Permission is hereby granted, free of charge, to any person obtaining a copy
		of this software and associated documentation files (the "Software"), to deal

[REFACTOR] Add and remove not useful person's fields #18

[REFACTOR] Add and remove not useful person's fields #18

Conversation

WilliamRyank commented Sep 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iskandarzulkarnaien commented Oct 1, 2019