Skip to content

[Nguyen Quang Anh] iP#202

Open
quanganh2810 wants to merge 25 commits into
nus-cs2113-AY2223S2:masterfrom
quanganh2810:master
Open

[Nguyen Quang Anh] iP#202
quanganh2810 wants to merge 25 commits into
nus-cs2113-AY2223S2:masterfrom
quanganh2810:master

Conversation

@quanganh2810

Copy link
Copy Markdown

No description provided.

@MustafaAH10 MustafaAH10 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.

Code is overall of good quality and has minimal errors.

Comment thread src/main/java/Duke.java Outdated
if (userCmd.equals("list")) {
listofItems.listTask();
} else if (WordsinUserCmd[0].equals("mark")) {
} else if (userCmdasWords[0].equals("mark")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

userCmdasWords is a bit confusing to reader.

Comment thread src/main/java/TaskManager.java Outdated
@@ -0,0 +1,40 @@
public class TaskManager {
private Task[] tasks = new Task[100];
private int tasksCount = 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.

Can be taskCount instead of tasksCount to follow naming convention.

Comment thread src/main/java/Task.java Outdated
@@ -0,0 +1,40 @@
public class Task {
private String name;
private boolean isDone;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Boolean name is good as status is intuitive.

Comment thread src/main/java/TaskManager.java Outdated
tasksCount++;
}

public void markTask(int id) {

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 consider combining markTask and unmarkTask into one function.

@kaceycsn kaceycsn 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.

Looks good! Code generally follows the coding standard. Some suggestions are given.

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

public void listTask() {
int j = 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.

Consider changing the variable name. Variables named j, k etc. should be used for nested loops only.

Suggested change
int j = 1;
int counter = 1;

Comment thread src/main/java/TaskManager.java Outdated
@@ -0,0 +1,40 @@
public class TaskManager {
private Task[] 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.

Great! Array specifiers is attached to the type not the variable.

Comment thread src/main/java/Duke.java Outdated
while (!userCmd.equals("bye")) {
String[] userCmdasWords = userCmd.split(" ");
System.out.println(userCmdasWords[0]);
if (userCmd.equals("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.

Perhaps you can consider using switch instead of multiple if-else statements to make your code cleaner and more readable

Comment thread src/main/java/Task.java Outdated
this.name = name;
}

public boolean IsDone() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider changing method name. Method names must be in camelCase.

@bdthanh bdthanh 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.

LGTM! However, you should careful with method and variable naming. I can just find several minor coding standard violations then good job! Good luck on your upcoming tasks

Comment thread src/main/java/duke/Event.java Outdated
Comment on lines +16 to +19
} else {
return " [E][ ]" + this.getName() + " (from: " + this.startTime
+ " to: " + this.finishTime + ")";
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

else block are unnecessary. You can move the code inside else block out and put it after if block. Same with other if else block

Comment thread src/main/java/duke/Event.java Outdated
}

public String toString() {
if(this.getIsDone() == true) {

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 should add space between if and condition statement

Comment thread src/main/java/Duke.java Outdated
Comment on lines +26 to +29
public static void falseInput() {
System.out.println("Sorry Duke could not understand your input :> please follow the instructions");
printInstructions();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Method's name should start with a verb for example announceFalseInput. Same with firstWord method below

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