[Zhao Luoyuang] iP #185

LS-Young · 2021-09-01T07:47:25Z

No description provided.

JMattChiam · 2021-09-03T04:37:37Z

src/main/java/Duke.java

@@ -22,7 +22,7 @@ public static void main(String[] args) {
            }else if(str.equals("list")){


Should have a space before and after the "else if" statements.

JMattChiam · 2021-09-03T04:43:03Z

src/main/java/Duke.java

+        String str = "Nice! I've marked this task as done: ";
+        int length = Math.max(t.getLength(), str.length());
+        System.out.print("      ");
+        for(int i = 0; i < length; i++) System.out.print("_");


For loop should not be one-line

JMattChiam · 2021-09-03T04:46:28Z

src/main/java/Duke.java

+        System.out.println(greetings);
+        Task[] Tasks = new Task[100];
+        Scanner sc= new Scanner(System.in); //System.in is a standard input stream
+        boolean flag = true;


Boolean variables should have boolean form, flag doesn't explain the variable

Agree with Matt. Consider using a name such as isExit, to indicate whether Duke should continue running or terminate. (if isExit is used, do remember to initialise isExit to false and change flag = true on line 21)

JMattChiam · 2021-09-03T04:50:08Z

src/main/java/Duke.java

+
+    public static void printList(Task[] list, int num){
+        String listing = "Here are the tasks in your list:";
+        if(listing.length() > num) num = listing.length();


if statement should not be a single line

JMattChiam · 2021-09-03T04:55:49Z

src/main/java/Duke.java

+        String greetings = " Hello! I'm Duke\n" + " What can I do for you?\n";
+        System.out.println(greetings);
+        Task[] Tasks = new Task[100];
+        Scanner sc= new Scanner(System.in); //System.in is a standard input stream


should find a better name for the variable instead of "sc"

DJ-Tan · 2021-09-03T06:44:07Z

src/main/java/Deadline.java

+    }
+    @Override
+    public int getLength(){
+        return description.length()+by.length()+12;


Consider leaving a space - description.length() + by.length() + 12
Also consider using a constant to define what 12 means

DJ-Tan · 2021-09-03T06:49:26Z

src/main/java/Duke.java

+        System.out.println(greetings);
+        Task[] Tasks = new Task[100];
+        Scanner sc= new Scanner(System.in); //System.in is a standard input stream
+        boolean flag = true;


Agree with Matt. Consider using a name such as isExit, to indicate whether Duke should continue running or terminate. (if isExit is used, do remember to initialise isExit to false and change flag = true on line 21)

DJ-Tan · 2021-09-03T06:50:38Z

src/main/java/Duke.java

+        System.out.println("Hello from\n" + logo);*/
+        String greetings = " Hello! I'm Duke\n" + " What can I do for you?\n";
+        System.out.println(greetings);
+        Task[] Tasks = new Task[100];


Consider using a constant such as private static final int MAX_TASK = 100 to represent 100. Similarly, try replacing all other magic numbers later on with constants

DJ-Tan · 2021-09-03T06:53:07Z

src/main/java/Duke.java

+                printTask(t, j);
+            }
+        }
+    }


Consider extracting code in each of the if-else blocks out to new methods that describe what check/extraction is being done

DJ-Tan · 2021-09-03T06:59:20Z

src/main/java/Duke.java

+        System.out.println("|");
+        System.out.print("     |");
+        for(int i = 0; i < length; i++) System.out.print("_");
+        System.out.println("|");
    }
 }


Overall, the code is fairly decent to follow. One general comment is that a lot of the methods can be extracted out to make it easier for somebody else to follow quickly. Single line if loops and for loops should still follow the format of:
if (CONDITION) {
// write code here
}
Consider using descriptive names for variables (exp: instead of str or str2, use errorMessage or helpMessage)

Tip: Try using CTRL+ALT+L if you are on windows to automatically indent your code in IntellIJ

DJ-Tan · 2021-09-03T07:01:10Z

src/main/java/Duke.java

+        System.out.print("      ");
+        for(int i = 0; i < length; i++) System.out.print("_");
+        System.out.println("");
+        System.out.print("     |" + str);


Considering that the indentation at the start of each string happens to always be " |", could try defining that particular string as a constant instead such as TAB

DJ-Tan · 2021-09-03T07:05:00Z

src/main/java/Duke.java

+                    Tasks[num - 1].setDone(true);
+                    printDone(Tasks[num - 1]);
+                } else {
+                    str = "not assigned yet";


The reuse of the str String variable may be non-intuitive - it initially refers to the command input by the user, and after re-assignment, it became the message back to the user. Consider using separate String names such as userCommand and returnMessage. Otherwise, just parse "not assigned yet" into printString()

* branch-Level-5: Use exceptions and edit code according to comments

* branch-A-Packages: use package to group exceptions and tasks

Leeyp

Good job on the code's logic and flow.

Do be careful about the requirements for coding standards and code quality. I recommend that you do a double check of your code for violations before the final submission.

Leeyp · 2021-09-17T14:35:29Z

src/main/java/Duke.java

+        int MAX_TASK = 100;
+        String greetings = " Hello! I'm Duke\n" + " What can I do for you?\n";
+        System.out.println(greetings);
+        //Task[] Tasks = new Task[MAX_TASK];


There should not be commented code at this stage of the project.

Leeyp · 2021-09-17T14:37:25Z

src/main/java/Duke.java

+        try {
+            File dukeFile = new File(file);
+            File directory = dukeFile.getParentFile();
+            if(!directory.exists()){


Coding standard violation: add spacebar after 'if' statement.

Leeyp · 2021-09-17T14:38:11Z

src/main/java/Duke.java

+            if(!directory.exists()){
+                directory.mkdir();
+            }
+            if(!dukeFile.exists()){


Coding standard violation: add spacebar after 'if' statement.

Additionally, consider abstracting this and the above into an external method.

Leeyp · 2021-09-17T14:39:17Z

src/main/java/Duke.java

+        Scanner sc= new Scanner(System.in); //System.in is a standard input stream
+        boolean isExit = true;
+        int maxlength = 0;
+        for(; Tasks.size() < 100 && isExit;){


Coding standard violation: add spacebar before bracket.

For loop should also follow Egyptian style brackets.

Leeyp · 2021-09-17T14:40:27Z

src/main/java/Duke.java

+        boolean isExit = true;
+        int maxlength = 0;
+        for(; Tasks.size() < 100 && isExit;){
+            String str= sc.nextLine();


There should be a spacebar after '=' sign.

Leeyp · 2021-09-17T14:40:57Z

src/main/java/Duke.java

+                    printList(Tasks, maxlength);
+                } else if (str.contains("todo")) {
+                    checkTodoString(str);
+                    if (str.length() - 5 + 9 > maxlength)


Avoid magic numbers. Is there any way to make this statement more legible?

Leeyp · 2021-09-17T14:41:40Z

src/main/java/Duke.java

+            } catch (TypingException e) {
+                  printString("OOPS! what do u mean?");
+            } catch (EmptyListException e) {
+                  printString("OOPS! empty list");


Try to add more enter key lines with this large else if statement for legibility.

Also, consider changing else if statements to a switch case statement.

Leeyp · 2021-09-17T14:42:56Z

src/main/java/Duke.java

+            switch (str.substring(0,1)) {
+            case "T":
+                String[] strT = str.split(" \\| ");
+                for(String st: strT) System.out.println(st);


The loop body should be wrapped by curly brackets irrespective of how many lines there are in the body.

Leeyp · 2021-09-17T14:43:29Z

src/main/java/Duke.java

+    }
+
+    public static void checkNum(int num, int j) throws AssignException {
+        if(num > j || num <= 0)


Avoid complicated expressions.

Add spacebar before brackets.

Same issue with the functions below.

stephlewyh · 2021-09-19T13:22:34Z

src/main/java/Duke.java

 public class Duke {
-    public static void main(String[] args) {
-        String logo = " ____        _        \n"
+    public static void main(String[] args) throws AssignException, TypingException, FormatException, EventStringException, DeadlineStringException, TodoStringException, IOException {


This main method is too long (>30 LoC). You may want to abstract out certain blocks of code within into methods and extend the SLAP principle to each task.

stephlewyh · 2021-09-19T13:24:01Z

src/main/java/Duke.java

+        for(int i = 0; i < Tasks.size(); i++){
+            switch (Tasks.get(i).getType()){
+            case TODO:
+                textToAdd += "T | " + (Tasks.get(i).isDone() ? "1" : "0") + " | " + Tasks.get(i).getDescription() + System.lineSeparator();


This line exceeds the conventional length of <120 characters per line. There are a few lines that are also doing the same thing. Perhaps reformatting the string with Java String format() method would help.

stephlewyh · 2021-09-19T13:26:01Z

src/main/java/Duke.java

+    public static void printString(String str){
+        int length = str.length();
+        System.out.print("      ");
+        for(int i = 0; i < length; i++) System.out.print("_");


The single line for statement is missing the curly brackets and line breaks required to meet the coding standard, which is at least 3 separate lines - they make code easier to read.

stephlewyh · 2021-09-19T13:27:44Z

src/main/java/duke/exception/TodoStringException.java

+package duke.exception;
+
+public class TodoStringException extends Exception {
+    //no other code needed


You could leave out this comment (and similar ones elsewhere) if it is not describing a method.

stephlewyh · 2021-09-19T13:28:36Z

src/main/java/duke/task/Todo.java

+        return isDone;
+    }
+    public String getStatusIcon() {
+        return (isDone ? "X" : " "); // mark done task with X


The comments could be placed on a separate line with same indentation. Refer to the Java coding standard for examples.

Branch level 9

Revert "Branch level 9"

LS-Young added 8 commits August 20, 2021 14:05

Level 0:greet

3d3a9a0

Greet, Echo, Exit

8641994

Add, List

7253b8d

Mark as Done

20918e3

try to follow coding standard

660e32f

add in functions to analyze "todo", "event" and "deadline" command

91955b5

input and EXPECTED edited

4aa000a

correct error and improve code quality

154a805

JMattChiam reviewed Sep 3, 2021

View reviewed changes

DJ-Tan reviewed Sep 3, 2021

View reviewed changes

LS-Young added 7 commits September 8, 2021 22:26

Use exceptions and edit code according to comments

a7b4a1c

Merge branch 'branch-Level-5'

586ba7e

* branch-Level-5: Use exceptions and edit code according to comments

use package to group exceptions and tasks

afd44a8

Merge branch 'branch-A-Packages'

8de5601

* branch-A-Packages: use package to group exceptions and tasks

Add delete function by using ArrayList

9ad898c

Store the task data in tasks.txt

c71229e

error corrected

13943e5

Leeyp reviewed Sep 17, 2021

View reviewed changes

stephlewyh reviewed Sep 19, 2021

View reviewed changes

LS-Young and others added 11 commits September 27, 2021 10:09

extract some classes

4df17cb

Ui, Storage, TaskList, Parser added

46aa54a

implement find function

a67be74

Merge pull request #1 from LS-Young/branch-Level-9

afba42f

Branch level 9

Revert "Branch level 9"

d37c10a

Merge pull request #2 from LS-Young/revert-1-branch-Level-9

cde6e2f

Revert "Branch level 9"

Merge branch 'branch-Level-9'

c9ad7b9

add comment to the classed and methods

0406861

Merge branch 'branch-A-JavaDoc'

6abea24

Set theme jekyll-theme-cayman

e41182e

Set theme jekyll-theme-cayman

b39c7ea

LS-Young and others added 5 commits September 30, 2021 09:50

fix

f2a5b85

Merge remote-tracking branch 'origin/master'

ffc4dc6

Update README.md

8504f51

error corrected

2d6b083

Merge branch 'master' of https://github.com/LS-Young/ip

e018577

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Zhao Luoyuang] iP #185

[Zhao Luoyuang] iP #185

LS-Young commented Sep 1, 2021

JMattChiam Sep 3, 2021

JMattChiam Sep 3, 2021

JMattChiam Sep 3, 2021

DJ-Tan Sep 3, 2021

JMattChiam Sep 3, 2021

JMattChiam Sep 3, 2021

DJ-Tan Sep 3, 2021

DJ-Tan Sep 3, 2021

DJ-Tan Sep 3, 2021

DJ-Tan Sep 3, 2021

DJ-Tan Sep 3, 2021

DJ-Tan Sep 3, 2021

DJ-Tan Sep 3, 2021

Leeyp left a comment

Leeyp Sep 17, 2021

Leeyp Sep 17, 2021

Leeyp Sep 17, 2021

Leeyp Sep 17, 2021

Leeyp Sep 17, 2021

Leeyp Sep 17, 2021

Leeyp Sep 17, 2021

Leeyp Sep 17, 2021

Leeyp Sep 17, 2021

Leeyp Sep 17, 2021

stephlewyh Sep 19, 2021

stephlewyh Sep 19, 2021

stephlewyh Sep 19, 2021

stephlewyh Sep 19, 2021

stephlewyh Sep 19, 2021

		@@ -22,7 +22,7 @@ public static void main(String[] args) {
		}else if(str.equals("list")){

[Zhao Luoyuang] iP #185

Are you sure you want to change the base?

[Zhao Luoyuang] iP #185

Conversation

LS-Young commented Sep 1, 2021

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

Leeyp 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

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