[Bui Gia Nhat Minh] iP by minhbgn · Pull Request #182 · nus-cs2113-AY2425S2/ip

minhbgn · 2025-02-06T08:35:52Z

No description provided.

wentuoc

Overall good readability with a clear and logial sequence! Just a few nits to pick. Good job :)

wentuoc · 2025-02-07T06:50:05Z

src/main/java/Main/Roboast.java

+
+        while (!input.equals("bye")) {
+
+            int space = input.indexOf(' ');


Would a more descriptive variable name such as spaceIndex be better?

wentuoc · 2025-02-07T06:52:28Z

src/main/java/Main/Roboast.java

+
+    private final ArrayList<Item> itemList = new ArrayList<Item>();
+
+    public void start() {


I like the use of abstraction to manage the different stages of the program!

wentuoc · 2025-02-07T06:56:11Z

src/main/java/Item/Deadline.java

+
+    public Deadline(String name, boolean done){
+        super(name, done);
+        String newname = name.replace("/by","(by:");


Should the by field be extracted out and stored as a separate attribute so that it can be accessed and modified when necessary? (e.g. say if I want to change the due date)

wentuoc · 2025-02-07T06:59:13Z

src/main/java/Item/Item.java

+
+public class Item {
+    protected String name;
+    protected boolean done;


Would it be better to use isDone as the name for this boolean variable?

wentuoc · 2025-02-07T07:00:41Z

src/main/java/Main/Roboast.java

+        while (!input.equals("bye")) {
+
+            int space = input.indexOf(' ');
+            boolean foundSpace = (space > 0);


I like the use of these boolean variables to simplify the expression in the if condition later on!

wentuoc · 2025-02-07T07:07:34Z

src/main/java/Main/Roboast.java

+        while (i < itemList.size()) {
+            if (itemList.get(i).isDone()){
+                itemList.remove(i);
+            }


Can consider adhering to K&R style for curly braces

yok2086 · 2025-02-07T09:13:24Z

src/main/java/Item/Item.java

+package Item;
+
+public class Item {
+    protected String name;


Could've considered using a more descriptive name for the String variable

yok2086 · 2025-02-07T09:14:50Z

src/main/java/Item/Item.java

+
+    @Override
+    public String toString() {
+        if (done){


Perhaps could've used a ternary operator instead?

yok2086 · 2025-02-07T09:16:46Z

src/main/java/Main/Roboast.java

+
+public class Roboast {
+    private static final String BOT_NAME = "Roboast";
+    private static final String LINE = "_".repeat(50);


I like how instead of typing the "_" out, you just used the repeat method.

yok2086 · 2025-02-07T09:17:49Z

src/main/java/Main/Roboast.java

+                String content = input.substring(space+1);
+                if (command.equals("setDone")){
+                    mark(content, true);
+                }


Maybe you should be more consistent with the spacing before the "{" if you want to add a space before it or not.

sevenseasofbri

Good job so far! You might need to work on following the layout accoridng to the java coding standard though.

sevenseasofbri · 2025-03-04T06:48:48Z

src/main/java/diskIO/CSVFileHandler.java

@@ -0,0 +1,57 @@
+package diskIO;


From java coding standard: Names representing packages should be in all lower case.

Consider making the package name something like storage or disk.io

sevenseasofbri · 2025-03-04T06:50:21Z

src/main/java/item/Deadline.java

+        super(name, done);
+        String newname = name.replace("/by","(by:");
+        int byIndex = newname.indexOf("(by:");
+
+        try {
+            deadline = newname.substring(byIndex + 5, newname.length() - 1);
+        }
+        catch (StringIndexOutOfBoundsException e) {
+            deadline = null;
+        }
+
+
+        if (!name.equals(newname)){
+            newname = newname + ")";
+        }
+        super.setItemName(newname);
+        super.itemType = "Deadline";
+    }
+
+    @Override
+    public String toString(){
+        if (super.isDone){
+            return "[D][X] " + super.itemName;
+        }
+        else{
+            return "[D][ ] " + super.itemName;


Perhaps avoid the usage of magic literals by using constants or vars.

sevenseasofbri · 2025-03-04T06:51:23Z

src/main/java/item/Event.java

+        try {
+            eventStartTime = newname.substring(newname.indexOf("(from:") + 6, newname.indexOf("to:") - 1);
+            eventEndTime = newname.substring(newname.indexOf("to:") + 4);;
+        }


try-catch blocks should ideally be of the form:

try { statements; } catch (Exception exception) { statements; }

sevenseasofbri · 2025-03-04T06:51:54Z

src/main/java/item/Event.java

+        if (super.isDone){
+            return "[E][X] " + super.itemName;
+        }
+        else{
+            return "[E][ ] " + super.itemName;
+        }


if-else clauses should be of the form:

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

sevenseasofbri · 2025-03-04T06:52:49Z

src/main/java/item/Event.java

@@ -0,0 +1,50 @@
+package item;
+
+public class Event extends Item{


Suggested change

public class Event extends Item{

public class Event extends Item {

sevenseasofbri · 2025-03-04T06:56:13Z

src/main/java/item/ItemManage.java

+        this.itemList = itemList;
+    }
+
+    public void action(String input) throws RoboastException {


This method is over >30 LOC. Consider refactoring the inner-levels of code into other methods.

Additionally, a method is supposed to be a verb. As such action does not fit this description. Consider calling it applyAction or something.

sevenseasofbri · 2025-03-04T06:58:26Z

src/main/java/item/ItemManage.java

+        try {
+            command = input.substring(0, spaceIndex);
+            content = input.substring(spaceIndex + 1);
+        }
+        catch (IndexOutOfBoundsException e) {
+            command = input;
+            content = "";
+        }
+        if (command.equals("mark")) {
+            mark(content, true);
+        }
+        else if (command.equals("unmark")) {
+            mark(content, false);
+        }
+        else if (command.equals("todo")) {
+            addTodo(content);
+        }
+        else if (command.equals("deadline")) {
+            addDeadlines(content);
+        }
+        else if (command.equals("event")) {
+            addEvents(content);
+        }
+        else if (command.equals("list")) {
+            showItemList();
+        }
+        else if (command.equals("deleteAll")) {
+            deleteAll();
+        }
+        else if (command.equals("delete")) {
+            delete(content);
+        }
+        else {
+            throw new RoboastException("Invalid command");
+        }


Again, the layout of these clauses needs to be adjusted (like mentioned before)

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

sevenseasofbri · 2025-03-04T07:00:22Z

src/main/java/item/ItemManage.java

+    public void addDeadlines(String content) {
+        if (content.isEmpty()){
+            showAddEmptyError();
+            return;
+        }
+        System.out.println(LINE);
+        Item item = new Deadline(content, false);
+        itemList.add(item);
+        System.out.println("added: " + item.getItemName() + "\n" +
+                "You now have " + itemList.size() + " tasks\n" + LINE);
+    }
+
+    public void addEvents(String content) {


The functions refer to events and deadline in plural, but the functions themselves add only 1 event or 1 deadline at a time. Consider naming them addDeadline and addEvent instead.

sevenseasofbri · 2025-03-04T07:02:14Z

src/main/java/item/ItemManage.java

+
+    }
+
+    public void mark(String content, boolean isDone) {


What is exaclt meant by variable content? Is it the input from the user? Consider naming it something else more intuitive.

sevenseasofbri · 2025-03-04T07:02:52Z

src/main/java/item/ItemManage.java

+            if (itemList.get(i).isDone()){
+                itemList.remove(i);
+            }
+            else{


Suggested change

else{

else {

Branch a user guide

minhbgn added 6 commits January 22, 2025 12:16

level-0

f73d18d

feat: Add Level-1

bb45e84

feat: Add Level-3

a64892c

feat: Level-4

8696843

feat: A-CodeQuality

20f057c

fix: Change method name mark to setDone

77bf3ef

wentuoc reviewed Feb 7, 2025

View reviewed changes

yok2086 reviewed Feb 7, 2025

View reviewed changes

minhbgn added 13 commits February 20, 2025 19:08

Add branch-Level-5

4f12a5d

Fix: Change message for clarity

5bcbb27

Feat: Merge Level-5 to main branch

c2a61f2

Fix: change ItemManage to Item package

9e6c536

Merge branch 'branch-A-Packages'

be633e1

Add Level-6

d83cd74

Add Level-6 in branch

87bcef3

Delete diskIO

3501472

Add Level-7

57c1e39

Merge branch 'branch-Level-6'

328ba40

Merge branch 'branch-Level-7'

6ac7741

Fix: Delete unknown files

cc96fc9

Merge branch 'branch-Level-7'

9b96998

sevenseasofbri reviewed Mar 4, 2025

View reviewed changes

minhbgn and others added 5 commits March 7, 2025 03:37

Add More OOP

fa6ecce

Add Level-9

abd4339

Add A-JavaDoc

e987ff2

Add A-UserGuide

353ac34

Merge pull request #1 from minhbgn/branch-A-UserGuide

c5007c0

Branch a user guide


		while (!input.equals("bye")) {

		int space = input.indexOf(' ');


		private final ArrayList<Item> itemList = new ArrayList<Item>();

		public void start() {

		@@ -0,0 +1,50 @@
		package item;

		public class Event extends Item{

	public class Event extends Item{
	public class Event extends Item {

Conversation

minhbgn commented Feb 6, 2025

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants