Skip to content

[Ong Jun Lin Jeremiah] iP#212

Open
miahjerry wants to merge 18 commits into
nus-cs2113-AY2223S2:masterfrom
miahjerry:master
Open

[Ong Jun Lin Jeremiah] iP#212
miahjerry wants to merge 18 commits into
nus-cs2113-AY2223S2:masterfrom
miahjerry:master

Conversation

@miahjerry

Copy link
Copy Markdown

No description provided.

Comment thread src/main/java/Duke.java Outdated
@@ -1,10 +1,73 @@
public class Duke {
import java.util.Scanner;
import java.util.*;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps you could list the imported classes explicitly

Comment thread src/main/java/Duke.java Outdated
Comment on lines +4 to +5
public class
Duke {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
public class
Duke {
public class Duke {

Comment thread src/main/java/Duke.java Outdated
Comment on lines +19 to +33
String logo = " ____ _ \n"
+ "| _ \\ _ _| | _____ \n"
+ "| | | | | | | |/ / _ \\\n"
+ "| |_| | |_| | < __/\n"
+ "|____/ \\__,_|_|\\_\\___|\n";
System.out.println("Hello from\n" + logo);

String greeting = ("____________________________________________________________\n"
+ "Hello! I'm Duke\n" + "What can I do for you?\n"
+ "____________________________________________________________\n"
);

String goodBye = ("____________________________________________________________\n"
+ "Bye. Hope to see you again soon!\n"
+ "____________________________________________________________\n");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps you can change your constant names to be all uppercase using underscore to separate words

Comment thread src/main/java/Duke.java Outdated
Comment on lines +61 to +62
System.out.println("OK, I've marked this task as not done yet:\n" + " "
+ toDoList.get(itemNumber).getStatusIcon() + toDoList.get(itemNumber).getDescription());

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think you did a good job indenting wrapped lines.

Comment thread src/main/java/Duke.java Outdated
Comment on lines +26 to +33
String greeting = ("____________________________________________________________\n"
+ "Hello! I'm Duke\n" + "What can I do for you?\n"
+ "____________________________________________________________\n"
);

String goodBye = ("____________________________________________________________\n"
+ "Bye. Hope to see you again soon!\n"
+ "____________________________________________________________\n");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You may want to refactor the greeting and goodbye into another function instead of putting it all in main(). For example a void printGreeting() function that just prints the greeting

Comment thread src/main/java/Task.java Outdated
public class Task {
protected String description;
protected boolean isDone;
protected int taskNumber;

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 are no methods that uses taskNumber?

Comment thread src/main/java/Duke.java Outdated
Comment on lines 19 to 24
String logo = " ____ _ \n"
+ "| _ \\ _ _| | _____ \n"
+ "| | | | | | | |/ / _ \\\n"
+ "| |_| | |_| | < __/\n"
+ "|____/ \\__,_|_|\\_\\___|\n";
System.out.println("Hello from\n" + logo);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can be refactored into print logo function, or you can combine it with the print greeting function

Comment thread src/main/java/Duke.java Outdated
ArrayList<Task> toDoList = new ArrayList<Task>(100);

while (!userInput.equals("bye")) {
if (!containsNumbers(userInput)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You probably don't need this if clause. Instead, you can see if the input string contains "list", "mark", or "unmark", and create 3 separate switch cases for them. Even better, you can refactor this entire if else block into a function that parses the user input.

Comment thread src/main/java/Duke.java Outdated
System.out.print(item.getStatusIcon());
System.out.println(item.getDescription());
}
System.out.println("____________________________________________________________\n");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can declare a constant for the row of underscores, or you can just create a function printLine to print the row of underscores. Also, you may want to use System.lineSeparator() instead of '\n' as recommended by the module.

Comment thread src/main/java/Duke.java Outdated
Comment on lines +46 to +48
System.out.print((toDoList.indexOf(item) + 1) + ".");
System.out.print(item.getStatusIcon());
System.out.println(item.getDescription());

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can be refactored into a separate function printTask(), since all tasks have the same format to be printed. In addition, depending on the situation, you may want to create a new function for each System.out.print() call, if you print the same thing multiple time.

Comment thread src/main/java/Duke.java Outdated
Comment on lines +4 to +5
public class
Duke {

@R-Ramana R-Ramana Feb 11, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please bring Duke { in line 5 to the same line as public class (line 4)

Suggested change
public class
Duke {
public class Duke {

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

public class
Duke {
public static void main(String[] args) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your main method is far too long, the recommendation according to the textbook is 30 LOC. Please try to modularize your main method.

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.

4 participants