Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MichelleLiang0116] iP #200

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

MichelleLiang0116
Copy link

No description provided.

@MichelleLiang0116 MichelleLiang0116 changed the title [ MichelleLiang0116] iP [MichelleLiang0116] iP Jan 31, 2023
Copy link

@skyanzy skyanzy left a comment

Choose a reason for hiding this comment

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

Good job! you did really well in following java coding standards! well done in making sure that no line exceeds 120 characters

Comment on lines 1 to 2
import java.util.ArrayList;
import java.util.Scanner;
Copy link

Choose a reason for hiding this comment

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

good job in importing classes explicitly

while (true) {
Scanner myObj = new Scanner(System.in);
instruction = myObj.nextLine();
if (instruction.equalsIgnoreCase("list")) {
Copy link

Choose a reason for hiding this comment

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

good job in using string.equalsIgnoreCase when comparing strings

Comment on lines 57 to 59
System.out.println(lineBreak + '\n' + "Got it. I've added this task:");
System.out.println('\t' + t.toString());
System.out.println("Now you have " + taskList.size() + " tasks in the list." + '\n' + lineBreak);
Copy link

Choose a reason for hiding this comment

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

good job in separating your output string to make your code look neat

Copy link

@Toh-HongFeng Toh-HongFeng left a comment

Choose a reason for hiding this comment

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

Good job on code quality, but can be improved.

instruction = myObj.nextLine();
if (instruction.equalsIgnoreCase("list")) {
System.out.println(lineBreak + '\n'
+ "Here are the tasks in your list:");

Choose a reason for hiding this comment

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

Executing list command can be done on another java file.

}
System.out.println(taskList.get(toMark - 1).toString() + '\n' + lineBreak);
} else {
Task t;

Choose a reason for hiding this comment

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

Naming for this variable can be clearer.

System.out.println(i + 1 + "." + taskList.get(i).toString());
}
System.out.println(lineBreak);
} else if (instruction.equalsIgnoreCase("bye")) {

Choose a reason for hiding this comment

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

Can add comments between each command's execution to improve readability.

@@ -0,0 +1,13 @@
public class Deadline extends Task {

Choose a reason for hiding this comment

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

Inheritance to split task types is well done.

@@ -0,0 +1,26 @@
public class Task {
private final String description;

Choose a reason for hiding this comment

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

Good job restricting access to object variables.

public class Duke {
private final static ArrayList<Task> taskList = new ArrayList<>();
static String lineBreak = "-----------------";

public static void main(String[] args) {

Choose a reason for hiding this comment

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

Your main method feels a bit long, can consider modularizing it. According to the textbook, the recommendation is to have 30 LOC.

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.

5 participants