Skip to content

[Shi Haochen] iP#77

Open
hshiah wants to merge 8 commits into
nus-cs2113-AY2324S1:masterfrom
hshiah:master
Open

[Shi Haochen] iP#77
hshiah wants to merge 8 commits into
nus-cs2113-AY2324S1:masterfrom
hshiah:master

Conversation

@hshiah

@hshiah hshiah commented Sep 5, 2023

Copy link
Copy Markdown

Shi Haochen's individual project update.

Comment thread src/main/java/Duke.java Outdated
String greet = "Hello! I'm Elwin\n" + "What can I do for you?";
String line = "_______________________________________________";
String exit = "Bye. Hope to see you again soon!";
String listRequirements = "Here are the tasks in your 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.

Good job! Use of camelCase

Comment thread src/main/java/Duke.java Outdated
+ "| |_| | |_| | < __/\n"
+ "|____/ \\__,_|_|\\_\\___|\n";
System.out.println("Hello from\n" + logo);
String greet = "Hello! I'm Elwin\n" + "What can I do for you?";

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 use capital letters for constant eg. GREET

Comment thread src/main/java/Duke.java Outdated
List<Task> todoList = new ArrayList<>();
while(true){
String input = scanner.nextLine();
if(input.equals("bye")){

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remember to add spacing for if else class statements eg. if (something) {

Comment thread src/main/java/Duke.java Outdated
@@ -1,10 +1,56 @@
import java.util.Scanner;
import java.util.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.

Good job! names representing packages are in lower case

Comment thread src/main/java/Task.java
@@ -0,0 +1,25 @@
public class Task {

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 job use of PascalCase for classes

@vvhuiling vvhuiling left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Overall, I found your code easy to read for the most part except main class appears to be a bit packed. Perhaps we can add some blank lines to improve readability. And perhaps encapsulate some of its functionalities if that make sense!

Comment thread src/main/java/Duke.java Outdated
todoList.get(index-1).markAsDone();
System.out.println(line);
System.out.println(markRequirements);
System.out.println(" [" + todoList.get(index-1).getStatusIcon() + "] " + todoList.get(index-1).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.

Perhaps spilt the code into 2 lines evenly? first line looks a bit to long for me. I noticed the same issue in several other places too.

Comment thread src/main/java/Duke.java Outdated
Comment on lines +12 to +13
String markFormat = "^mark \\d+$";
String unmarkFormat = "^unmark \\d+$";

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 have some confusion regarding these strings. Can you provide some clarification on their meaning?

Comment thread src/main/java/Duke.java Outdated
Comment on lines +19 to +35
while(true){
String input = scanner.nextLine();
if(input.equals("bye")){
break;
}else if(input.equals("list")){
System.out.println(line);
System.out.println(listRequirements);
for(int i = 0; i < todoList.size(); i++){
System.out.println(i+1 + ".[" + todoList.get(i).getStatusIcon() + "] " + todoList.get(i).getDescription());
}
System.out.println(line);
}else if(input.matches(markFormat)) {
int index = Integer.parseInt(input.substring(5));
todoList.get(index-1).markAsDone();
System.out.println(line);
System.out.println(markRequirements);
System.out.println(" [" + todoList.get(index-1).getStatusIcon() + "] " + todoList.get(index-1).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.

Perhaps encapsulate some of its functionalities into separate methods so that this while loop can be more concise.

Comment thread src/main/java/Duke.java Outdated
System.out.println("Hello from\n" + logo);
String greet = "Hello! I'm Elwin\n" + "What can I do for you?";
String line = "_______________________________________________";
String exit = "Bye. Hope to see you again soon!";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since this line is used only once, should it be extracted out and just print it directly when needed?

@maanyos maanyos left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Generally good adherence to coding standards. Some variable names can be improved to be more informative.
Good use of execeptions

Comment thread src/main/java/Duke.java Outdated
import java.util.List;
import java.util.ArrayList;
public class Duke {
protected final static String line = "_______________________________________________";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

follow the correct naming standard for constants

Comment thread src/main/java/Duke.java Outdated
+ "|____/ \\__,_|_|\\_\\___|\n";
System.out.println("Hello from\n" + logo);
String greet = "Hello! I'm Elwin\n" + "What can I do for you?";
//String line = "_______________________________________________";

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 unecessary code

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