Skip to content

[Jiayan Ben] iP#76

Open
J-Y-Yan wants to merge 87 commits into
nus-cs2113-AY2324S1:masterfrom
J-Y-Yan:master
Open

[Jiayan Ben] iP#76
J-Y-Yan wants to merge 87 commits into
nus-cs2113-AY2324S1:masterfrom
J-Y-Yan:master

Conversation

@J-Y-Yan

@J-Y-Yan J-Y-Yan commented Sep 5, 2023

Copy link
Copy Markdown

No description provided.

@J-Y-Yan J-Y-Yan changed the title Add a empty file "Hello.txt" [Jiayan Ben] IP Sep 5, 2023
@J-Y-Yan J-Y-Yan changed the title [Jiayan Ben] IP [Jiayan Ben] iP Sep 5, 2023

@StanleyW00 StanleyW00 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 nice.
Just need to fix some of the names.

Comment thread src/main/java/Duke.java Outdated
Comment on lines +6 to +7
Task[] list = new Task[100];
int count = 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.

Perhaps the variable names should be renamed specifically to task in order to avoid confusion in the future.

Suggested change
Task[] list = new Task[100];
int count = 0;
Task[] tasks = new Task[100];
int tasksCount = 0;

Comment thread src/main/java/Task.java Outdated
Comment on lines +14 to +20
public void Done(){
this.isDone = true;
}

public void unDone(){
this.isDone = false;
}

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 these methods should be renamed to represent verbs.

Suggested change
public void Done(){
this.isDone = true;
}
public void unDone(){
this.isDone = false;
}
public void markAsDone(){
this.isDone = true;
}
public void unmarkAsDone(){
this.isDone = false;
}

Comment thread src/main/java/Duke.java Outdated
Comment on lines +45 to +47
if( (task > count ) || (task <1) ){
System.out.println("Oops! You don't have the task in this positions.");
}

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 like that you checked whether the task position is there or not.

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

case "todo":
// command e.g. todo borrow book

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 like that you put the example inputs for each type of tasks.

Comment thread src/main/java/Deadline.java Outdated
@@ -0,0 +1,14 @@
public class Deadline extends Task {

protected String by;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Confusing variable name, use noun for variables.

Suggested change
protected String by;
protected String userName;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for your suggestions. I have improved the naming for a few variables to avoid confusion.

Comment thread src/main/java/Duke.java Outdated
Comment on lines +13 to +14
String command = scanner.nextLine();
String[] parts = command.split(" ");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unspecific variable naming.

Suggested change
String command = scanner.nextLine();
String[] parts = command.split(" ");
String userCommand = scanner.nextLine();
String[] userInputParts = userCommand.split(" ");

Comment thread src/main/java/Todo.java Outdated
Comment on lines +8 to +11
public String toString() {
return "[T]" + super.toString() ;
}
}

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 use of inheritance in implementation of toString().

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

case "mark":
int taskNo = Integer.parseInt(parts[1]); //assume saved in part[1], need further improve

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Comment could be more specific, and dumb downed for the reader instead of for own purpose.

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

case "deadline":
// command e.g. deadline return book /by Sunday
String[] ddlSplit = command.split("/");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Avoid using short forms as variable names as it might confuse readers with different backgrounds.

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

while (true) {
String command = keyboard.nextLine();
String[] commendSplits = command.split(" ");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Small typo on "commendSplits"

Comment thread src/main/java/Task.java Outdated
Comment on lines +15 to +19
if( (taskNo > taskCount ) || (taskNo <1) ){
System.out.println("Oops! You don't have any task in this positions.");
}else if(this.isDone){
System.out.println("You have already completed the task.");
} else{

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Spacing of the if else statements does not follow standards. Similar in other areas

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