[Shulong] iP by DreamerDragon · Pull Request #467 · nus-cs2103-AY2021S1/ip

DreamerDragon · 2020-08-30T15:51:02Z

Working in process

dhafinrazaq

Hi, those are my PR review (on coding standard)!
If I mentioned "Reference", it means it was taken from the Java coding standard from CS2103T website.

dhafinrazaq · 2020-09-01T02:35:39Z

+        int location = 0;
+        boolean exist = false;
+        for (int i = 0; i < string.length(); i++){
+            if(string.charAt(i) == '/'){


Should there be whitespace after "if"? I noticed the same issue in several other places too.
Reference: "Java reserved words should be followed by a white space." from CS2103 Java coding standard web page

Thanks Dahfin, I will change that!

dhafinrazaq · 2020-09-01T02:36:18Z

+    // Find out the position of the first '/'
+    public int firstSlashTracker (String string){
+        int location = 0;
+        boolean exist = false;


Should the name of the boolean be "isExist"? I also noticed the same issue in other boolean variables too.
Reference: "Boolean variables/methods should be named to sound like booleans"

good point!

dhafinrazaq · 2020-09-01T02:39:56Z

@@ -1,10 +1,268 @@
+import java.util.ArrayList;


Hi, should there be a package statement above this line?
Reference: "Every class should be part of some package."

so how does your package look like?

it actually depends on how the directory in your project looks like. one of my package statements is "package command"

dhafinrazaq · 2020-09-01T02:44:28Z

+                        System.out.println("oops, the deadline description cannot be empty");
+                    }else {
+                        int number = firstSlashTracker(order);
+                        Task item = new Task(1, 0,order.substring(8, number) + "(by: " +


Should the plus sign at the end of the line be placed below instead (i.e. after the break)?
Reference: "Break before an operator."

ohh I will take note !

dhafinrazaq · 2020-09-01T02:47:46Z

@@ -0,0 +1,10 @@
+public class Task {
+    public int status;


Should this be declared as non-public? I also see the same issue on some other variables.
Reference: "Class variables should never be declared public unless the class is a data class with no behavior."

behaviour here means methods?

I'm not 100% sure, but I think it means methods

yuxuanxc

The code seems to be working fine and satisfies the requirements of the iP. The quality of the code in terms of readability could be improved on, and try to keep your if/else and try/catch blocks as concise as possible. Jiayou for the rest of your iP!

yuxuanxc · 2020-09-01T14:41:51Z

+            var correct = true;
+            var deleted = false;
+            var done = order.length() >= 4 && order.substring(0 , 4).equals("done");
+            var delete = order.length() >= 6 && order.substring(0 , 6).equals("delete");
+            var todo = order.length() >= 4 && order.substring(0 , 4).equals("todo");
+            var deadline = order.length() >= 8  && order.substring(0, 8).equals("deadline");
+            var event = order.length() >= 5 && order.substring(0, 5).equals("event");


Maybe you can consider specifying the data type of your variables instead of just using 'var' for all of them? I'm not sure about the technical benefits of using int, boolean, String etc but it would definitely improve the readability of your code which will be helpful for your tP! Also, it seems like all of them are boolean values so you could consider naming them isCorrect, isDeleted, isInputDone etc to follow the coding standard.

yuxuanxc · 2020-09-01T14:49:14Z

+            var done = order.length() >= 4 && order.substring(0 , 4).equals("done");
+            var delete = order.length() >= 6 && order.substring(0 , 6).equals("delete");
+            var todo = order.length() >= 4 && order.substring(0 , 4).equals("todo");
+            var deadline = order.length() >= 8  && order.substring(0, 8).equals("deadline");
+            var event = order.length() >= 5 && order.substring(0, 5).equals("event");


Not sure if you already know but order.substring(0 , 4).equals("done") could be replaced with order.startsWith("done"), just to improve readability but you will get the same result either way.

yuxuanxc · 2020-09-01T15:01:23Z

+                        if(orders.get(i).status == 0) {
+                            if(orders.get(i).category == 0){
+                                System.out.println(i + 1 + ". " + " [T][x] "+ orders.get(i).command);
+                            }else if(orders.get(i).category == 1){
+                                System.out.println(i + 1 + ". " + " [D][x] " + orders.get(i).command);
+                            }else if(orders.get(i).category == 2){
+                                System.out.println(i + 1 + ". " + " [E][x] " + orders.get(i).command);
+                            }else {
+                                System.out.println(i + 1 + ". " + " [x] " + orders.get(i).command);
+                            }
+                        }else {
+                            if(orders.get(i).category == 0){
+                                System.out.println(i + 1 + ". " + " [T][v] "+ orders.get(i).command);
+                            }else if(orders.get(i).category == 1){
+                                System.out.println(i + 1 + ". " + " [D][v] " + orders.get(i).command);
+                            }else if(orders.get(i).category == 2){
+                                System.out.println(i + 1 + ". " + " [E][v] " + orders.get(i).command);
+                            }else {
+                                System.out.println(i + 1 + ". " + " [v] " + orders.get(i).command);
+                            }
+                        }


One possible way to make the printing of the tasks out neater could be by overriding the toString method of your Task class so that you can just call System.out.println(i + 1 + ". " + orders.get(i)) instead. It is the same logic as what you are doing here, just that the checking of status and category is done inside the Task class and this will help with abstraction.

yuxuanxc · 2020-09-01T15:14:20Z

+            if(correct && !done && !deleted && !order.equals("task list")) {
+                if(todo){
+                    if(order.length() <= 4){
+                        System.out.println("oops, the todo description cannot be empty");
+                    }else {
+                        Task item = new Task(0, 0, order.substring(4) );
+                        orders.add(item);
+                    }
+                }else if(deadline){
+                    if(order.length() <= 8){
+                        System.out.println("oops, the deadline description cannot be empty");
+                    }else {
+                        int number = firstSlashTracker(order);
+                        Task item = new Task(1, 0,order.substring(8, number) + "(by: " +
+                                order.substring(number + 3) + ")");
+                        orders.add(item);
+                    }
+                }else if(event){
+                    if(order.length() <= 5){
+                        System.out.println("oops, the event description cannot be empty");
+                    }else {
+                        int number = firstSlashTracker(order);
+                        Task item = new Task(2, 0, order.substring(5, number) + "(at: " +
+                                order.substring(number + 3) + ")");
+                        orders.add(item);
+                    }
+
+                }else {
+                    Task item = new Task(4, 0, order);
+                    orders.add(item);
+                }
+
+            }


I'm not sure why you decided to make another if block to check for todo, event, and deadline since you already checked for the booleans at lines 117, 126 and 137. Would combining the bodies of the 2 if blocks of todo change the output in any way?

yuxuanxc · 2020-09-01T15:25:47Z

@@ -0,0 +1,10 @@
+public class Task {
+    public int status;


Does the status(0, 1) refer to whether or not the task is done? If that's the case, changing it to a boolean(eg. isDone) will be helpful in terms of readability. It might reduce confusion as your teammates will not have to guess which variable does what when they read your code for tP/ other group projects in the future.

yuxuanxc · 2020-09-01T15:29:21Z

+    public String command;
+    Task(int category, int status, String command){
+        this.category = category;
+        this.status = status;


The status of the task is always set to 0 when you initialize a new Task object, so the constructor may not have to take in a parameter for status and this.status = 0; could be used instead?

yuxuanxc · 2020-09-01T15:53:01Z

+        try {
+            File myFile = new File(filePath);
+            Scanner myReader = new Scanner(myFile);
+            while (myReader.hasNextLine()) {
+                String data = myReader.nextLine();
+                System.out.println(data);
+            }
+            myReader.close();
+        } catch (FileNotFoundException e) {
+            System.out.println("Creating the file to save");
+            try {
+                String folderPath = "./data";
+                Path path = Paths.get(folderPath);
+
+                Files.createDirectories(path);
+
+                System.out.println("Directory is created!");
+
+                //Files.createDirectory(path);
+
+            } catch (IOException error) {
+                System.err.println("Failed to create directory!" + error.getMessage());
+            }
+            try {
+                File fileCreator = new File(filePath);
+                if (fileCreator.createNewFile()) {
+                    System.out.println("File created: " + fileCreator.getName());
+                } else {
+                    System.out.println("File already exists.");
+                }
+            } catch (IOException ex) {
+                System.out.println("An error occurred.");
+                e.printStackTrace();
+            }
+        }
+        try {
+            File myFile = new File(filePath);
+            Scanner myReader = new Scanner(myFile);
+            while (myReader.hasNextLine()) {
+                String data = myReader.nextLine();
+                System.out.println(data);
+            }
+            myReader.close();
+        } catch (FileNotFoundException e) {
+            System.out.println("Creating the file to save");
+        }
+
+        try {
+            FileWriter myWriter = new FileWriter(filePath);
+            //myWriter.write("Files in Java might be tricky, but it is fun enough!");
+            myWriter.close();
+            System.out.println("All changes successfully saved to the storage!");
+        } catch (IOException e) {
+            System.out.println("An error occurred.");
+            e.printStackTrace();
+        }


I'm sorry but I don't really understand this part. Would it be possible to combine all the try blocks together into one and have multiple catch blocks for different errors instead?

chuyouchia

In general, the code has some further progress to make. I think you would be on the right track after trying to refactor the code as mentioned.

Potentially, you could implement an iterative approach to this. Instead of trying to solve all the problems at once, try to categorize the issues into UI related, encapsulation or abstraction issues. Try solving them one by one and it might be easier to implement in that respect since many other reviewers are also leaving their comments. Hope that helps you!

chuyouchia · 2020-09-02T13:31:55Z

+                break;
+            }
+        }
+        if(exist) {


There are some recurring inconsistencies with usage of white space. It might be good to review to keep it consistent with the rest of the code base.

for example: if (exist) would better adhere to coding styles than if(exist)

chuyouchia · 2020-09-02T13:35:11Z

+    // A-trial run
+    // program to be added
+    // Find out the position of the first '/'
+    public int firstSlashTracker (String string){


Names representing methods must be verbs and written in camelCase. For example, getName(), computeTotalWidth()

So, if the intention is for you to get the value of first slash tracker, the method name could be getFirstSlashTracker instead

chuyouchia · 2020-09-02T13:43:41Z

+                    System.out.println(" Yup, this is now added! " + " [E][x] " +  order.substring(5, number) + "(at: " +
+                            order.substring(number + 3) + ")");


It seems like you are making a line break in order to follow the rule of line length not exceeding 120 and that is great!

However, line breaks should be done before an operator. One possible way could be to change this line to the following format :
System.out.println(" Yup, this is now added! " + " [E][x] "

order.substring(5, number) + "(at: "

order.substring(number + 3) + ")");

the operator should be a part of the new line when wrapping lines. Hope this example clarifies 👍

Wow! Thank you for pointing it!!

Branch a assertions

Branch a code quality

C find duplicates

damithc and others added 11 commits July 23, 2020 23:27

Add Gradle support

3b19ba1

Level-1

cc6c420

Level-2

f13b06c

Level-3

2fae636

Level-4

1492099

A-TextUiTesting

da4753a

Level-5

974e054

Level-6

8adb9f2

Level-7

0f7a149

merge branch-level-7

52f0b98

Level-8

02639bc

dhafinrazaq reviewed Sep 1, 2020

View reviewed changes

yuxuanxc reviewed Sep 1, 2020

View reviewed changes

chuyouchia reviewed Sep 2, 2020

View reviewed changes

Shulong added 16 commits September 2, 2020 22:10

A-MoreOOP

df5b0b0

A-JUnit

17e5b38

A-Jar

e9da59c

A-JavaDoc

9e01d9f

A-CodingStandard

da712d0

Level-9

bdc38e7

A-JavaDoc

782a52b

A-CodingStandard

c0e5707

merge branch-A-JavaDoc

258d6a3

merge confilct

9aed463

current version

1cb9417

merge gardle files

d2a5a1c

fixed merge with gradle

a842d79

A-Gradle

56f75db

Level-10

89551fe

A-Assertions

3b5b52d

Shulong and others added 21 commits September 9, 2020 02:00

A-CodeQuality

20b255c

Merge pull request #1 from DreamerDragon/branch-A-Assertions

479fca4

Branch a assertions

updated master branch

ce7907f

Merge pull request #2 from DreamerDragon/branch-A-CodeQuality

a8122c7

Branch a code quality

BCD-Extension

3bf5aa3

Merge branch 'master' into C-findDuplicates

abe3628

Merge pull request #3 from DreamerDragon/C-findDuplicates

9944c9e

C find duplicates

A-BetterGui

928832c

A-UserGuide

173d7bb

A-betterGui

e5a4502

updates master branch with Parser and ParserGUI classes

b6cd165

A-BetterGui

6dab289

Set theme jekyll-theme-architect

ccf2246

Finalise KaTo

92c2ab3

add in docs/_config.yml

df831bb

Added clear command in ParserGui and GUI class

c40eb7a

Update UG with the clear feature

2080a0e

Added javaDoc for the clear command

6bf9cec

Fixed import spacing issues

0ce5803

removed clear command from the user guide

21dc446

removed clear command from the user guide

81272a5

		System.out.println(" Yup, this is now added! " + " [E][x] " + order.substring(5, number) + "(at: " +
		order.substring(number + 3) + ")");

Conversation

DreamerDragon commented Aug 30, 2020

Uh oh!

dhafinrazaq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuxuanxc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuxuanxc Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chuyouchia left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yuxuanxc Sep 1, 2020 •

edited

Loading