[Abhitya Krishnaraj] iP by abhityakrishnaraj · Pull Request #65 · nus-cs2113-AY2223S1/ip

abhityakrishnaraj · 2022-08-30T09:41:18Z

No description provided.

winston-lim

Other then some spacing and naming issues, looks good to me!

winston-lim · 2022-08-31T04:32:28Z

+
+public class Deadline extends Task{
+    protected String date;
+    public Deadline(String n, boolean d, String dat){


Consider having more semantically meaningful name for parameters

winston-lim · 2022-08-31T04:33:30Z

+
+    @Override
+    public String toString() {
+        return "[D]"+super.toString()+" (by: "+date+")";


Add spaces between binary operators and their operands "[D]" + super.toString() + " (by: " + date + ")"

winston-lim · 2022-08-31T04:36:09Z

+
+        System.out.println("Hello! I'm Duke");
+        System.out.println("What can I do for you");
+        String inp = in.nextLine();


Single comment for whole file:

Consider not shortening variable names, especially if its already short enough < 8 chars

inp, comm, tas requires context to understand so it may be better to leave them as is or name differently

winston-lim · 2022-08-31T04:36:39Z

+
+    @Override
+    public String toString() {
+        return "[E]"+super.toString()+" (at: "+date+")";


Same as other file, consider adding spaces between operands and binary operators

winston-lim · 2022-08-31T04:37:09Z

+        super(n, d);
+    }
+    public String toString(){
+        return "[T]"+super.toString();


Same as above, add spaces

bdthanh

To add space, you can use shortcut Ctrl + Alt + L

bdthanh · 2022-08-31T04:34:54Z

@@ -0,0 +1,14 @@
+package main.java;
+
+public class Deadline extends Task{


Add space between Class name and bracket

bdthanh · 2022-08-31T04:35:41Z

+
+public class Deadline extends Task{
+    protected String date;
+    public Deadline(String n, boolean d, String dat){


Add space between function name and bracket

bdthanh · 2022-08-31T04:36:03Z

+        System.out.println("What can I do for you");
+        String inp = in.nextLine();
+        ArrayList<Task> list = new ArrayList<>();
+        while(!inp.equals("bye")){


bdthanh · 2022-08-31T04:36:20Z

+        String inp = in.nextLine();
+        ArrayList<Task> list = new ArrayList<>();
+        while(!inp.equals("bye")){
+            if(inp.equals("list")) {


Add space after if

bdthanh · 2022-08-31T04:37:30Z

+        }
+    }
+
+    public String toString() {


Should add @OverRide

bdthanh · 2022-08-31T04:37:59Z

+    public Todo(String n, boolean d){
+        super(n, d);
+    }
+    public String toString(){


Should override the to String function in Task

bdthanh · 2022-08-31T04:40:30Z

+    public Task(String n, boolean d) {
+        name = n;
+        isDone = d;
+    }


public Task(String name, boolean isDone) { this.name = name; this.isDone = isDone; }

richwill28

Some parts could be improved. Please pay more attention to SLAP and separation of concerns.

richwill28 · 2022-09-17T10:19:54Z

@@ -0,0 +1,206 @@
+package main;
+
+import java.io.*;


[Coding Standard Violation] Imported classes should always be listed explicitly.

richwill28 · 2022-09-17T10:20:06Z

+import java.io.*;
+import java.nio.file.Paths;
+import java.nio.file.Path;
+import java.util.*;


[Coding Standard Violation] Imported classes should always be listed explicitly.

richwill28 · 2022-09-17T10:24:17Z

+        String logo = " ____        _        \n"
+                + "|  _ \\ _   _| | _____ \n"
+                + "| | | | | | | |/ / _ \\\n"
+                + "| |_| | |_| |   <  __/\n"
+                + "|____/ \\__,_|_|\\_\\___|\n";
+        System.out.println("Hello from\n" + logo);
+
+        System.out.println("Hello! I'm Duke");
+        System.out.println("What can I do for you");


[Code Quality] Consider extracting UI-related constructs to a dedicated class.

richwill28 · 2022-09-17T10:26:18Z

+        System.out.println("Hello! I'm Duke");
+        System.out.println("What can I do for you");
+        fileToList();
+        String inp = in.nextLine();


[Code Quality] inp is not very friendly, consider changing the variable name.

Tip: use intention-revealing name.

richwill28 · 2022-09-17T10:33:33Z

+        while(!inp.equals("bye")){
+            if(inp.equals("list")) {
+                for (int i = 1; i <= list.size(); i++) {
+                    System.out.print(i + ". ");
+                    System.out.println(list.get(i - 1));
+                }
+            }
+            else if (inp.contains("mark")){
+                int num = Integer.parseInt(inp.substring(inp.indexOf(" ")+1))-1;
+                if(num>list.size()){
+                    System.out.println("Out of bounds of the list");
+                }
+                else{
+                    if(inp.contains("unmark")){
+                        System.out.println("I have marked this task as not done yet");
+                        list.get(num).setDone(false);
+                    }
+                    else{
+                        System.out.println("I have marked this task as complete");
+                        list.get(num).setDone(true);
+                    }
+                    System.out.println(list.get(num));
+                    listToFile();
+                }
+
+            }
+            else if(inp.contains("delete")){
+                int num = Integer.parseInt(inp.substring(inp.indexOf(" ")+1))-1;
+                if(num>list.size()){
+                    System.out.println("Out of bounds of the list");
+                }
+                else{
+                    System.out.println("I have removed this task: ");
+                    System.out.println(list.get(num));
+                    list.remove(num);
+                    System.out.println("You now have "+list.size()+" tasks left in the list.");
+                    listToFile();
+                }
+            }
+            else {
+                try{
+                    taskReader(inp);
+                    listToFile();
+                }
+                catch (DukeException e){
+                    e.printError();
+                }
+            }
+            inp = in.nextLine();
+        }


[Code Quality] This whole section is hard to read and reason about.

The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that.

Tip: extract the entire logic to a dedicated Parser class.

richwill28 · 2022-09-17T10:39:42Z

+            try{
+                if(!(f.createNewFile())){
+                    f.delete();
+                    f = new File("docs/duke.txt");
+                }
+            }
+            catch(IOException e){
+
+            }


[Code Quality] The try-catch here is unnecessary. Try to figure out why?

richwill28 · 2022-09-17T10:47:34Z

+    public static void fileToList(){
+
+        File f = new File("docs/duke.txt");
+
+        try{
+            if(!(f.createNewFile())){
+                Scanner sc = new Scanner(f);
+                while(sc.hasNextLine()) {
+                    String line = sc.nextLine();
+                    String[] lin = line.split(",");
+                    Boolean temp;
+                    temp = lin[1].equals("True");
+                    if(lin[0].equals("T")){
+                        list.add(new Todo(lin[2], temp));
+                    }
+                    else if(lin[0].equals("D")){
+                        list.add(new Deadline(lin[2], temp, lin[4]));
+                    }
+                    else if(lin[0].equals("E")){
+                        list.add(new Event(lin[2], temp, lin[4]));
+                    }
+                }
+            }
+        }
+        catch (IOException e){
+            System.out.println("Problem occurred with the file");
+        }
+    }


[Code Quality] Again, the method fileToList seems to be a utility function. Consider extracting it out.

richwill28 · 2022-09-17T11:01:29Z

@@ -0,0 +1,206 @@
+package main;


Duke.java

There are several coding standard violations that I did not point out. Please refer to https://se-education.org/guides/conventions/java/index.html.

Overall, the design for class Duke is not very good, e.g. unrelated static methods, SLAP is lacking, readability issues.

richwill28 · 2022-09-17T11:05:13Z

+    }
+    public void printError() {
+        if (name.equals("")){
+            if (type.equals("todo")||type.equals("deadline")||type.equals("event")) {


[Code Quality] Consider extracting out this complicated expression.

richwill28 · 2022-09-17T11:08:07Z

+
+public class Event extends Task{
+    protected String date;
+    public Event(String n, boolean d, String dat){


[Code Quality] Consider renaming the method parameters.

Level-9 Merge pull request from branch

branch-A-JavaDoc

…avaDoc

abhityakrishnaraj and others added 4 commits August 25, 2022 16:13

Stage-0

832c8b3

Level-1

f93aca6

Level-2

3eefd74

Level-3

0ec75cb

abhityakrishnaraj changed the title ~~Abhitya Krishnaraj iP~~ [Abhitya Krishnaraj] iP Aug 30, 2022

Level-4

1207e8a

winston-lim reviewed Aug 31, 2022

View reviewed changes

bdthanh reviewed Aug 31, 2022

View reviewed changes

abhityakrishnaraj added 4 commits September 6, 2022 21:42

Level-5

3892000

Level-6

c5aec36

Level-7

6ee37f9

A-Jar

98672d7

richwill28 reviewed Sep 17, 2022

View reviewed changes

abhityakrishnaraj added 11 commits September 30, 2022 15:26

A-moreOOP

f86d760

Level-9

41caa77

Merge pull request #1 from abhityakrishnaraj/branch-level-9

b6e295d

Level-9 Merge pull request from branch

branch-JavaDoc

38de6d5

Merge pull request #2 from abhityakrishnaraj/branch-JavaDoc

66c2bcc

branch-A-JavaDoc

A-Jar

6784fb4

More final fixes

8f8256a

Merge commit '66c2bcc2e402ee9fd7c44eb4a230e2a6697f7f88' into branch-J…

19be715

…avaDoc

A-UserGuide

c87ef76

Update README.md

e1df1bb

Update README.md

8ab1c85

		@@ -0,0 +1,14 @@
		package main.java;

		public class Deadline extends Task{

Uh oh!

Conversation

abhityakrishnaraj commented Aug 30, 2022

Uh oh!

winston-lim 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!

bdthanh 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!

richwill28 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants