Skip to content

[Ahmish Nair] iP#179

Open
Ahmish15 wants to merge 25 commits into
nus-cs2113-AY2425S2:masterfrom
Ahmish15:master
Open

[Ahmish Nair] iP#179
Ahmish15 wants to merge 25 commits into
nus-cs2113-AY2425S2:masterfrom
Ahmish15:master

Conversation

@Ahmish15
Copy link
Copy Markdown

@Ahmish15 Ahmish15 commented Feb 6, 2025

No description provided.

Copy link
Copy Markdown

@H-ZhanHao H-ZhanHao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code was overall very neat and readable. Some nits to fix to make it even better.

Comment thread src/main/java/TaskManager.java Outdated
private int taskCount;

public TaskManager(){
tasks = new Task[100];
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic number used. A constant may help with readability in the future.

Comment thread src/main/java/chatbot.java Outdated
public class chatbot {
public static void main(String[] args) {
Scanner in = new Scanner(System.in);
TaskManager taskManager = new TaskManager();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to create a class for task management

Comment thread src/main/java/chatbot.java Outdated
int index = Integer.parseInt(input.substring(5)) - 1;
taskManager.mark(index);
} else if (input.startsWith("unmark ")) {
int index = Integer.parseInt(input.substring(7)) - 1;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get index could be abstracted into a function to make the code more readable here.

Comment thread src/main/java/chatbot.java Outdated
Comment on lines +33 to +36
String[] parts = input.substring(6).split("/from|/to");
String description = parts[0].trim();
String from = parts[1].trim();
String to = parts[2].trim();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the input filtering be abstracted to make this part more readable?

Copy link
Copy Markdown

@NCF3535 NCF3535 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the code is pretty clean readable, good job! Do try to use more OOP and note the coding standard violations to improve your code readably even more. Keep up the good work!

Comment thread data/duke.txt.rej Outdated
@@ -0,0 +1,8 @@
diff a/data/duke.txt b/data/duke.txt (rejected hunks)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove these unused files and use .gitignore so these files don't show up on GitHub. Alternatively, you may always use the version control to retrieve old versions of the code.

Comment thread src/main/java/TaskManager.java Outdated

public TaskManager(){
tasks = new Task[100];
taskCount = 0;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Donot use magic numbers in the code. You can replace it with a constant so that it will improve readability.

Comment thread src/main/java/TaskManager.java Outdated

private void loadFromFile() {
try {
File f = new File(FILE_PATH);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a better variable name as compared to f to improve readability.

Comment thread src/main/java/TaskManager.java Outdated

Task t;
switch (type) {
case "T":
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be no indentation for case clauses for switch cases unless you are using Lambda-style switch statements.

Comment thread src/main/java/chatbot.java Outdated
}

try {
if (input.equalsIgnoreCase("list")) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This area is very deeply nested and has a lot of repetition. You could probably use a switch to prevent this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants