Skip to content

[Wang Zhihuang] iP#181

Open
zh1huang wants to merge 39 commits intonus-cs2113-AY2122S1:masterfrom
zh1huang:master
Open

[Wang Zhihuang] iP#181
zh1huang wants to merge 39 commits intonus-cs2113-AY2122S1:masterfrom
zh1huang:master

Conversation

@zh1huang
Copy link
Copy Markdown

@zh1huang zh1huang commented Sep 1, 2021

No description provided.

Tweaked code to comply with coding standard
Added javadocs for non-trivial methods and classes
Added logo
Comment thread src/main/java/Task.java Outdated
public String getAt() {
return "";
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It might be better to have a blank line among each function.

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 agree. It may be minor but it can help make the functions look more distint and clear.

Copy link
Copy Markdown

@izdiyadfrhn izdiyadfrhn left a comment

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, except the parts where more distinction and separation could have been made between blocks of code. Otherwise, looks good!

Comment thread src/main/java/Ui.java Outdated
Comment on lines +12 to +21
public void start() {
String lineBreak = " ____________________________________________________________\n";
String logo = "\n" +
" \n" +
" ,--. ,------. ,--. ,--. ,--. \n" +
",-' '-.,---.| .-. \\ ,---.| | `--',---,-' '-. \n" +
"'-. .-| .-. | | \\ | .-. | | ,--( .-'-. .-' \n" +
" | | ' '-' | '--' ' '-' | '--| .-' `)| | \n" +
" `--' `---'`-------' `---'`-----`--`----' `--' \n" +
" \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.

interesting twist to the Duke java graphic provided with the starting template!

Comment thread src/main/java/TaskManager.java Outdated
Comment on lines +76 to +80
if (getSize() == 1) {
System.out.println(" Now you have " + 1 + " task in the list.");
} else {
System.out.println(" Now you have " + getSize() + " tasks in the 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.

Minor style and language taken care of. Nice!

Comment thread src/main/java/Task.java Outdated
public String getAt() {
return "";
}

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 agree. It may be minor but it can help make the functions look more distint and clear.

Comment thread src/main/java/TaskManager.java Outdated
Comment on lines +19 to +37
switch(taskType) {
case "todo":
newTask = new ToDo(description);
taskList.add(newTask);
break;
case "deadline":
String by = getDate(task);
newTask = new Deadline(description, by);
taskList.add(newTask);
break;
case "event":
String at = getDate(task);
newTask = new Event(description, at);
taskList.add(newTask);
break;
default:
System.out.println(" Invalid command, please try again");
return;
}
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 make this block of code separate from the few lines above it?

Comment thread src/main/java/Duke.java
Comment on lines +5 to +8
TaskManager taskManager = new TaskManager();
Scanner scanner = new Scanner(System.in);
Ui ui = new Ui(taskManager, scanner);
ui.start();
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 how you created a taskManager class to handle the commands and an Ui class to handle all the output, making the code very easy to understand.

Comment thread src/main/java/Task.java Outdated
Comment on lines +14 to +22
public String getStatusIcon() {
return (isDone ? "[X]" : "[ ]");
}
public String getIcon() {
return "";
}
public String getTiming() {
return "";
}
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 would be cleaner to add a new line between each function?

Comment thread src/main/java/TaskManager.java Outdated
String taskType = getCommand(task);
Task newTask;
String description = getDescription(task);
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.

I like how you named the variables, very intuitive and easy to understand.

Comment thread src/main/java/Ui.java Outdated
System.out.print(lineBreak);
}
}
}
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 really like how you have broken down everything into classes. I think it is very good OOP. Perhaps add some javadocs to explain some of the methods?

Copy link
Copy Markdown

@Leeyp Leeyp left a comment

Choose a reason for hiding this comment

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

Generally well done, keep up the good work!

Added some comments as room for improvements.

Comment thread src/main/java/duke/command/Ui.java Outdated
private static void saveData() {
try {
String pathName = PATH_NAME;
//create folder with file if absent initially
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 adding a line spacing before a comment for improved readability.

Comment thread src/main/java/duke/data/Storage.java Outdated

while (scanner.hasNext()) {
String line = scanner.nextLine();
String[] array = line.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.

Might want to improve on the name for "array" to make it more meaningful.

String timing = task.getTime();

textToAppend = taskType + " | " + status + " | " + description;
if (task instanceof Event || task instanceof Deadline) {
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 complicated expressions, is there any way you can extract it out or make this more streamlined? (hint: use booleans)

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