Tissue type field #96

SimNee · 2019-10-11T08:34:40Z

Added tissue type field and its test file. It passed travis and gradle as of now

coveralls · 2019-10-11T08:37:05Z

Coverage increased (+0.4%) to 75.635% when pulling 9d15055 on tissuetype-field into 1179f90 on master.

C-likethis123 · 2019-10-11T08:38:36Z

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

+        } else {
+            for (int i = 0; i < tissuetypeValue.length; i++) {
+                try {
+                    Integer tt = Integer.parseInt(tissuetypeValue[i]);


There is no check for whether the tissue types are numbers from 1-12

can you add check that all numbers is non duplication and from range 1-12.

Actually according to the link jiaying gave and google, there seems to be more than 12 types of tissue. But I cant really find a fixed number online on how manu types exactly. Should we just assume its 12?

I did some additional research. Apparently there are a lot more tissue types. We can stick to 12 tissues.

Ok noted. Will change by tonight. I can make to 20 actually since I think the total type is more than 20

just stick with 12 for simplicity. Thanks!

so just have a check if all number is <= 12 and there is non duplicate

Can add comment that its > 12 types but we are limiting it for now. Perhaps add //TODO: comment

C-likethis123 · 2019-10-11T08:41:31Z

src/test/java/seedu/address/model/person/TissuetypeTest.java

+    }
+
+    @Test
+    public void isValidTissuetype() {


Just wondering, if I have an input string "1, 2, 3, 4, 5, 6", would it count?

yes, line 38 tested it

I meant if the input string will accept the inputs separated by a comma and a space. Line 38 tests if the inputs are separated by commas.

C-likethis123 · 2019-10-11T08:42:48Z

src/test/java/seedu/address/model/person/TissuetypeTest.java

+        assertFalse(Tissuetype.isValidTissuetype("1 2 3 4 5 6")); // spaces within tissuetype
+
+        // valid tissuetype
+        assertTrue(Tissuetype.isValidTissuetype("1,2,3,4,5,6")); // exactly 6 tissuetype


Could you include test cases for tissue types > 12? Eg 13, 14.

yes, line 45

Line 45 tests the toString() method, not the isValidTissueType() method. Could you add test cases which checks for numbers that are not accepted as valid tissue types?

C-likethis123

Good work! I've requested some changes.

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

iskandarzulkarnaien · 2019-10-12T15:42:34Z

If the PR fixes a certain issue please tag it by adding fixes #issueNumber in the description

WilliamRyank

LGTM

C-likethis123 · 2019-10-14T01:46:38Z

src/main/java/organice/model/person/Tissuetype.java

+            for (int i = 0; i < tissuetypeValue.length; i++) {
+                try {
+                    Integer tt = Integer.parseInt(tissuetypeValue[i]);
+                    if (tt > 12) {


How about a check for tt < 1?

Yes, please check for < 1. Can check together with > 12 in same line.

C-likethis123

Overall good job, do make the changes requested.

iskandarzulkarnaien · 2019-10-14T03:16:15Z

src/main/java/organice/model/person/Bloodtype.java

    public final String value;

    /**
     * Constructs a {@code Bloodtype}.
     *
-     * @param bloodtype A valid bloodtype.


Change to camelCase for all references of BloodType or bloodType ?

Ctrl + shift +R for find and replace

iskandarzulkarnaien · 2019-10-14T03:16:52Z

src/main/java/organice/model/person/Tissuetype.java

+ * Represents a Person's tissue type in ORGANice.
+ * Guarantees: immutable; is valid as declared in {@link #isValidTissuetype(String)}
+ */
+public class Tissuetype {


CamelCase here as well ?

iskandarzulkarnaien · 2019-10-14T03:22:24Z

src/main/java/organice/model/person/Tissuetype.java

+        String[] tissuetypeValue = test.split(",");
+        if (tissuetypeValue.length != 6 || isDuplicated(tissuetypeValue)) {
+            return false;
+        } else {


Can we drop the else block to reduce indentation by 1 level? What does the code style say about this?

iskandarzulkarnaien · 2019-10-14T03:22:33Z

src/main/java/organice/model/person/Tissuetype.java

+            for (int i = 0; i < tissuetypeValue.length; i++) {
+                try {
+                    Integer tt = Integer.parseInt(tissuetypeValue[i]);
+                    if (tt > 12) {


Yes, please check for < 1. Can check together with > 12 in same line.

iskandarzulkarnaien · 2019-10-14T03:26:29Z

src/main/java/organice/model/person/Tissuetype.java

+    /**
+     * Returns true if a given string array contains duplicate.
+     */
+    public static boolean isDuplicated(String[] test) {


Perhaps hasDuplicates or containsDuplicates would be a better name

iskandarzulkarnaien · 2019-10-14T03:28:47Z

src/test/java/organice/model/person/TissuetypeTest.java

+        // invalid tissuetype
+        assertFalse(Tissuetype.isValidTissuetype("")); // empty string
+        assertFalse(Tissuetype.isValidTissuetype(" ")); // spaces only
+        assertFalse(Tissuetype.isValidTissuetype("a,b,c,d,e,f")); // wrong tissuetype


Perhaps would be better to say \\ tissue types must be Integers from 1..12. The dots are intentional btw. .. means range inclusive, ... means range exclusive

C-likethis123

LGTM

iskandarzulkarnaien

LGTM. Thanks @SimNee & @WilliamRyank

iskandarzulkarnaien · 2019-10-15T02:05:38Z

Added tissue type field and its test file. It passed travis and gradle as of now

In the future please use the issue description to describe the issue. Leave comments such as travis and gradle to a follow up comment. Thanks

iskandarzulkarnaien · 2019-10-15T04:12:42Z

Added tissue type field and its test file. It passed travis and gradle as of now

In the future please use the issue description to describe the issue. Leave comments such as travis and gradle to a follow up comment. Thanks

This is due to the fact that when the issue is referenced elsewhere, your description will be shown when people hover over the link

tissue type field

934b4d8

C-likethis123 reviewed Oct 11, 2019

View reviewed changes

C-likethis123 requested changes Oct 11, 2019

View reviewed changes

WilliamRyank reviewed Oct 11, 2019

View reviewed changes

src/main/java/seedu/address/model/person/Tissuetype.java Outdated Show resolved Hide resolved

test for duplicates and invalide tissuetypes more than 12

7009f08

WilliamRyank approved these changes Oct 12, 2019

View reviewed changes

WilliamRyank added 2 commits October 14, 2019 00:59

Merge branch 'master' into tissuetype-field

a50f8ee

Change package name

a2baf6a

SimNee requested a review from C-likethis123 October 14, 2019 01:35

C-likethis123 reviewed Oct 14, 2019

View reviewed changes

C-likethis123 requested changes Oct 14, 2019

View reviewed changes

iskandarzulkarnaien requested changes Oct 14, 2019

View reviewed changes

Fix camel case

9d15055

C-likethis123 approved these changes Oct 14, 2019

View reviewed changes

iskandarzulkarnaien approved these changes Oct 14, 2019

View reviewed changes

WilliamRyank merged commit 8cf997f into master Oct 14, 2019

iskandarzulkarnaien mentioned this pull request Oct 15, 2019

Add Tissue Type (Donor) #58

Closed

WilliamRyank deleted the tissuetype-field branch October 15, 2019 05:54

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

Feedback on week 9 project progress for CS2103T-T13-1 #118

Open

26 tasks

Tissue type field #96

Tissue type field #96

Conversation

SimNee commented Oct 11, 2019

coveralls commented Oct 11, 2019 • edited Loading

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

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

C-likethis123 left a comment

Choose a reason for hiding this comment

iskandarzulkarnaien commented Oct 12, 2019 • edited Loading

WilliamRyank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

C-likethis123 left a 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

C-likethis123 left a comment

Choose a reason for hiding this comment

iskandarzulkarnaien left a comment

Choose a reason for hiding this comment

iskandarzulkarnaien commented Oct 15, 2019

iskandarzulkarnaien commented Oct 15, 2019

coveralls commented Oct 11, 2019 •

edited

Loading

iskandarzulkarnaien commented Oct 12, 2019 •

edited

Loading