[Kim Gyurim] iP by yok2086 · Pull Request #188 · nus-cs2113-AY2425S2/ip

yok2086 · 2025-02-06T17:54:27Z

No description provided.

ElonKoh

Overall, I found your code easy to read with the correct syntaxes and your variables are named appropriately. I made some minor notes to improve readability, best practice suggestions, and consistency issues. Good work!

ElonKoh · 2025-02-07T03:40:51Z

src/main/java/David.java

+        Scanner scanner = new Scanner(System.in);
+        String input = scanner.nextLine();
+        int i = 0;
+        char taskType = input.charAt(input.indexOf("[") + 1);


Could this variable (taskType) be better named to indicate it being a part of the user input?

ElonKoh · 2025-02-07T03:42:25Z

src/main/java/David.java

+                        + "added: " + input + "\n" + LINE_SEPERATOR);
+                task[i] = new Task(input);
+                i++;
+            }


I like how readable this code logic for your while loop is, abstracting out all the print methods :)

src/main/java/David.java

ElonKoh · 2025-02-07T03:51:08Z

src/main/java/David.java

+        System.out.println(LINE_SEPERATOR + "\n" + "[" + task[index].getStatusIcon() + "] " + task[index].getDescription() + "\n" + LINE_SEPERATOR);
+    }
+
+    public static int getIndex(String marking, String find) {


I like that you abstracted this bit of code and named your variables well with a verb and camelCase to make the main method really readable

ElonKoh · 2025-02-07T04:12:35Z

src/main/java/Task.java

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


I like how you write comments to explain code 🤌🏻
but it could be better to explain why instead of what the code is doing

ElonKoh · 2025-02-07T04:27:27Z

src/main/java/David.java

+import java.util.Scanner;
+
+public class David {
+    public static final String LINE_SEPERATOR = "____________________________________________________________";


I like how you named this static final variable in ALL_CAPS

ElonKoh · 2025-02-07T04:28:23Z

src/main/java/Task.java

+
+    @Override
+    public String toString() {
+        return "[" + getStatusIcon() + "]"; // You can customize this string as needed


what is the message you intend to convey with this comment?

H-ZhanHao

Overall nice and understandable code! Just some nits to fix to make it even better.

H-ZhanHao · 2025-02-07T17:50:35Z

src/main/java/David.java

+                + "| |_| | |_| |   <  __/\n"
+                + "|____/ \\__,_|_|\\_\\___|\n";
+        System.out.println("Hello from\n" + logo);
+        Task[] task = new Task[100];


Magic number. Perhaps a constant would be more suitable?

H-ZhanHao · 2025-02-07T17:59:29Z

src/main/java/David.java

+                System.out.println(printTaskType(task, i));
+                i++;
+            } else if (input.startsWith("event")) {
+                task[i] = new Event((input.substring(getIndex(input, " ") + 1, getIndex(input, "/") - 1)), input.substring(getIndex(input, "from") + 5, getIndex(input, "to") - 2), input.substring(getIndex(input, "to") + 3));


Seems like the string filtering takes up a lot of space. Maybe abstracting it into a separate method would make this more readable.

H-ZhanHao · 2025-02-07T18:02:32Z

src/main/java/David.java

+import java.util.Scanner;
+
+public class David {
+    public static final String LINE_SEPERATOR = "____________________________________________________________";


Great idea to make the line a constant.

H-ZhanHao · 2025-02-07T18:04:26Z

src/main/java/David.java

+                }
+            } else if (input.startsWith("mark")) {
+                markTask(task, Integer.parseInt(input.split(" ")[1]) - 1);
+                printMark(task, Integer.parseInt(input.split(" ")[1]) - 1);


markTask and printMark both take the same arguments. Could it be better to combine the two?

This reverts commit f9fc435.

sevenseasofbri

Good job so far! You can improve readability by reducing deep indentation and refactoring long methods into smaller ones.

sevenseasofbri · 2025-03-05T09:21:31Z

src/main/java/David.java

+
+        Scanner scanner = new Scanner(System.in);
+        String input = scanner.nextLine();
+        int i = task.size();


Avoid naming non-loop iterator variables names such as i since this is not inuitive. Rather name this variable something like numTasks

sevenseasofbri · 2025-03-05T09:21:59Z

src/main/java/David.java

+        Scanner scanner = new Scanner(System.in);
+        String input = scanner.nextLine();
+        int i = task.size();
+        char input_type = input.charAt(input.indexOf("[") + 1);


Avoid the use of magic literals by storing them in variables or consts

sevenseasofbri · 2025-03-05T09:23:01Z

src/main/java/David.java

+public class David {
+    public static final String LINE_SEPERATOR = "____________________________________________________________";
+
+    public static void main(String[] args) {


This main function is quite long, >30 LOC. Consider breaking it into parts and put them in different functions.

sevenseasofbri · 2025-03-05T09:24:04Z

src/main/java/David.java

+                if (input.equals("list")) {
+                    System.out.println(LINE_SEPERATOR + "\nHere are the tasks in your list:\n");
+                    for (int j = 0; j < task.size(); j++) {
+                        System.out.println((j + 1) + "." + task.get(j).toString() + "\n" + LINE_SEPERATOR);
+                    }
+                }


This is 4 levels of indentation, so this can be deemed arrowhead code. Try to refactor the inner levels into methods.

sevenseasofbri · 2025-03-05T09:26:34Z

src/main/java/David.java

+                else if (input.startsWith("mark")) {
+                    markTask(task, Integer.parseInt(input.split(" ")[1]) - 1);
+                    printMark(task, Integer.parseInt(input.split(" ")[1]) - 1);
+                    StorageManager.saveTasks(task);
+                } else if (input.startsWith("unmark")) {
+                    unmarkTask(task, Integer.parseInt(input.split(" ")[1]) - 1);
+                    printUnmark(task, Integer.parseInt(input.split(" ")[1]) - 1);
+                    StorageManager.saveTasks(task);


Good job keeping a consistent level of abstraction here.

sevenseasofbri · 2025-03-05T09:29:34Z

src/main/java/David.java

+
+                }
+                else if (input.startsWith("delete")) {
+                    deleteTask(task, Integer.parseInt(input.split(" ")[1]) - 1);
+                    StorageManager.saveTasks(task);
+                    i--;
+                }
+
+                else {
+                    throw new IllegalArgumentException(LINE_SEPERATOR + System.lineSeparator() + " Sorry... I don't know what you mean by that? Could you try again?" + System.lineSeparator() + LINE_SEPERATOR);
+                }
+            }
+         catch (IllegalArgumentException e) {


if-else conditionals should be of the form (layout-wise):

if (condition) { statements; } else if (condition) { statements; } else { statements; }

sevenseasofbri · 2025-03-05T09:29:52Z

src/main/java/David.java

+    }
+    private static String printTaskType(ArrayList<Task> task, int i) {


try to have atleast 1 line space between methods.

sevenseasofbri · 2025-03-05T09:42:29Z

src/main/java/David/StorageManager.java

+    static {
+        File folder = new File("data");
+        if (!folder.isDirectory() && !folder.mkdirs()) {
+            System.err.println("Failed to create directory: " + folder.getAbsolutePath());
+        }
+    }


Since this is a class-level member, this means every instance of StorageManager will access the same data folder. Theoretically speaking, it is ok for this application. But it might make more sense for each instance of this StorageManager to access their own folders. Just a thought, consider looking into the textbook sections on OOP and class-level members.

sevenseasofbri · 2025-03-05T09:43:07Z

src/main/java/David/StorageManager.java

+    }
+
+    public static ArrayList<Task> loadTasks() {
+        File file = new File(FILE_PATH); // create a File for the given file path


No need to add a comment if it is obvious through the code.

sevenseasofbri · 2025-03-05T09:45:09Z

src/main/java/David/StorageManager.java

+        try (BufferedReader reader = new BufferedReader(new FileReader(file))) {
+            String line;
+            while ((line = reader.readLine()) != null) {
+                String[] parts = line.split(" \\| ");
+                if (parts.length < 3) {
+                    System.out.println("Skipping invalid task entry: " + line);
+                    continue;


this is ~3 levels of indentation, consider refactoring inner indents to another method

Level 9. Find

* branch-A-JavaDoc: A-JavaDoc : Add JavaDoc Comments # Conflicts: # src/main/java/David/Parser.java # src/main/java/David/TaskList.java

A-UserGuide: User Guide

yok2086 added 6 commits February 7, 2025 01:36

Level-0. Rename, Greet, Exit

bb2d95a

Level-1: Echo

8a51432

Level 2: Add, List

1579809

Level-3: Mark as Done

5c1f7d5

A-CodeStandard

adca4dd

Level 4: ToDo, Events, Deadlines

303d7cb

ElonKoh reviewed Feb 7, 2025

View reviewed changes

A-TextUiTesting

7d56a7d

H-ZhanHao reviewed Feb 7, 2025

View reviewed changes

yok2086 added 11 commits February 8, 2025 21:41

A-CodeQuality

f9fc435

Revert "A-CodeQuality"

4f9f867

This reverts commit f9fc435.

Committed A-Code-Quality code in Level 5 Branch

317a6ea

Level 5. Handle Errors

1b29810

Created David Exceptions to handle errors

8d42585

A-Packages : Organize into packages

5f0320f

Level-6 Delete

ec1266d

Level-7 Save

6411425

A-Jar

0abe3f1

Deleted Comments

08a7e42

Fixed Load Data Delete Issues

d46c87b

sevenseasofbri reviewed Mar 5, 2025

View reviewed changes

yok2086 and others added 9 commits March 13, 2025 00:53

A-MoreOOP : Use More OOP

c4fd83f

Level 9. Find

5c2c1e4

Merge pull request #1 from yok2086/branch-Level-9

fef593d

Level 9. Find

A-JavaDoc : Add JavaDoc Comments

35fcfcf

Merge branch 'branch-A-JavaDoc'

3e4dc79

* branch-A-JavaDoc: A-JavaDoc : Add JavaDoc Comments # Conflicts: # src/main/java/David/Parser.java # src/main/java/David/TaskList.java

A-UserGuide: User Guide

e7b24da

Merge pull request #3 from yok2086/branch-A-UserGuide

5608655

A-UserGuide: User Guide

Added Image to David ReadMe

a4b0571

Fixed ReadMe Format

fe8fe33

yok2086 added 2 commits March 13, 2025 19:02

Fixed Minor Unmark/Mark Tasks Bug

f1b149a

Fixed Image Issue in Readme

68630ae

		}
		private static String printTaskType(ArrayList<Task> task, int i) {

Conversation

yok2086 commented Feb 6, 2025

Uh oh!

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

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!

H-ZhanHao 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!

sevenseasofbri 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