Skip to content

[Hu Wenqi] iP - #95

Open
Vinci-Hu wants to merge 50 commits into
nus-cs2113-AY2021S2:masterfrom
Vinci-Hu:master
Open

[Hu Wenqi] iP#95
Vinci-Hu wants to merge 50 commits into
nus-cs2113-AY2021S2:masterfrom
Vinci-Hu:master

Conversation

@Vinci-Hu

@Vinci-Hu Vinci-Hu commented Feb 3, 2021

Copy link
Copy Markdown

No description provided.

Implement an initial skeletal version of the Duke that simply greets the user and exits.
Simply echos commands entered by the user, and exits when the user types "bye".
Add the ability to store whatever text entered by the user and display them back to the user when requested.
Add the ability to mark tasks as done.
Assuming all input are correct format

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

There are some coding standard violations, but overall good quality code!

Comment thread src/main/java/Duke.java Outdated
public static String readPrompt(Scanner in){
return in.nextLine();
}
/*

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe the comment can be removed, as we are not using it in the code.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah is this a leftover from some debugging you were doing? please remove

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

private static void addToList(Task[] tasks, String prompt, int taskIndex, String taskType) {
int cutOffPoint = 0;
switch(taskType) {

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 removing indentations for the case clauses. (It violates coding standard)

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.

Yes, you are right. Noted.

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

public static void main(String[] args) {
String logo = " ____ _ \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.

Consider removing the logo variable can be deleted. (It is not used in the rest of the code)

Comment thread src/main/java/Task.java Outdated
public String toString() {
return "[" + getStatusIcon() + "] " + 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.

Maybe it is better to remove the extra line spacings.

Comment thread src/main/java/Event.java
this.at = at;
}

@Override

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 to state @OverRide!

Comment thread src/main/java/Duke.java Outdated
public static String readPrompt(Scanner in){
return in.nextLine();
}
/*

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yeah is this a leftover from some debugging you were doing? please remove

Comment thread src/main/java/Task.java
this.isDone = false;
totalTasks ++;
}
//"\u2713" = tick symbol, "\u2718" = cross symbol

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 of you to comment this so that readers can know what the unicode symbols are

Comment thread src/main/java/Duke.java Outdated
tasks[taskIndex].markAsDone();
System.out.println("\tNice! I've marked this task as done: \n" +
"\t" + tasks[taskIndex].toString());
//bug here: does not deal with overflow values of input

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice comment. Mentioning possible bugs for reviewers is great

Comment thread README.md Outdated
1. Select the project directory, and click `OK`
1. If there are any further prompts, accept the defaults.
1. After the importing is complete, locate the `src/main/java/Duke.java` file, right-click it, and choose `Run Duke.main()`. If the setup is correct, you should see something like the below:
1. After the importing is complete, locate the `src/main/java/duke.Duke.java` file, right-click it, and choose `Run duke.Duke.main()`. If the setup is correct, you should see something like the below:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actual java filenames is conventionally the Java class name (just Duke). This could be a little complex.

Comment thread src/main/java/duke/Duke.java Outdated
import java.util.Iterator;
import java.util.Scanner;

import static duke.FileManager.*;

@stephlewyh stephlewyh Feb 21, 2021

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Are there more specific classes from FileManager that you would be importing? It would be good to state them explicitly here.

Comment thread src/main/java/duke/Duke.java Outdated
Comment on lines +171 to +174
Task newTodo = new Todo(description.substring(1));
taskList.add(newTodo);
writeToFile(initializeTextToWrite());
printAddSuccessMessage(newTodo);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Here, there are two methods being used, except that steps to task creation and addition to task list are explicitly written. You could consider incorporating SLAP.

Comment thread src/main/java/duke/Duke.java Outdated
throw new DukeException("Task does not exist");
}
Task toBeDeleted = taskList.get(taskIndex);
// Some manipulation here

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This comment is a little ambiguous. You could consider describing the below lines a little more. Alternatively, leaving it could be better.

Comment thread src/main/java/duke/Duke.java Outdated
System.out.print(DIVLINE + GREETINGS + DIVLINE);
Scanner in = new Scanner(System.in);
while (!isExiting) {
try{

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 ensuring spaces before {, to make the layout more consistent.

Comment thread src/main/java/duke/Duke.java Outdated
//create tasklist according to file storage
String[] arr = fileInput.get(i).split("\\|");
switch (arr[0]) {
case "T":

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using a single alphabets to describe cases does not provide greater clarity on what cases the method is handling. You could see Using names to explain for alternatives.

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