-
Notifications
You must be signed in to change notification settings - Fork 482
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
[Tang-Moyan] iP #542
base: master
Are you sure you want to change the base?
[Tang-Moyan] iP #542
Conversation
3df8678
to
e5419c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall ok and neat code that is readable. Could consider more encapsulation and java doc comments also! But I think it is fine and nice.
src/main/java/Duke.java
Outdated
import java.io.IOException; | ||
import java.nio.file.Path; | ||
import HelperClass.Task; | ||
|
||
public class Duke { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, not sure which level you are at but you could consider more encapsulation of code 😸
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for your review! I am a bit falling behind in catching up with the levels. I will try to get them done sooner.
src/main/java/Duke.java
Outdated
printOneLine(); | ||
} | ||
|
||
public static void Exit() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double spacing can change to single between public and static
src/main/java/Duke.java
Outdated
System.out.println("Directory '" + directoryName + "' created."); | ||
} else { | ||
System.err.println("Failed to create directory '" + directoryName + "'."); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant return statement
src/main/java/Duke.java
Outdated
|
||
printOneLine(); | ||
switch (userInput) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block of code is kindda big so maybe can encapsulate out
@@ -0,0 +1,3 @@ | |||
todo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can try to be a bit more comprehensive
src/main/java/HelperClass/Task.java
Outdated
public class Task { | ||
private boolean isDone; | ||
|
||
private int type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can consider a more clear name beside type for int
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job on your progress thus far ! A-CheckStyle increment will come in really handy for catching styling errors !
src/main/java/Duke.java
Outdated
switch (parser.getCommand()) { | ||
|
||
case "bye": | ||
//bye | ||
|
||
wantToExit = true; | ||
ui.Speak(ui.Exit()); | ||
|
||
break; | ||
|
||
case "list": | ||
//list | ||
ui.Speak(tasks.displayList()); | ||
break; | ||
|
||
case "mark": | ||
//mark 1 | ||
|
||
try { | ||
int i = Integer.parseInt(parser.getTaskName()) - 1; | ||
ui.Speak(tasks.markTask(i)); | ||
userListHaveChanges = true; | ||
} catch (NumberFormatException e) { | ||
ui.Speak("need to provide an integer index of task."); | ||
} | ||
|
||
|
||
break; | ||
|
||
case "unmark": | ||
//unmark 1 | ||
try { | ||
int i = Integer.parseInt(parser.getTaskName()) - 1; | ||
ui.Speak(tasks.unmarkTask(i)); | ||
userListHaveChanges = true; | ||
} catch (NumberFormatException e) { | ||
ui.Speak("need to provide an integer index of task."); | ||
} | ||
|
||
break; | ||
|
||
case "todo": | ||
//todo read book | ||
ui.Speak(tasks.addTask(new Task(parser.getTaskName(), | ||
1, "Null", "Null", false))); | ||
userListHaveChanges = true; | ||
break; | ||
|
||
case "deadline": | ||
//deadline read book /by 2022-01-01 | ||
ui.Speak(tasks.addTask(new Task(parser.getTaskName(), | ||
2, "Null", parser.getFirstEnteredTime(), false))); | ||
userListHaveChanges = true; | ||
break; | ||
|
||
case "event": | ||
//event read book /from 2022-01-01 /to 2022-01-02 | ||
ui.Speak(tasks.addTask(new Task(parser.getTaskName(), | ||
3, parser.getFirstEnteredTime(), parser.getSecondEnteredTime(), false))); | ||
userListHaveChanges = true; | ||
break; | ||
|
||
case "delete": | ||
//delete 1 | ||
try { | ||
int i = Integer.parseInt(parser.getTaskName()) - 1; | ||
ui.Speak(tasks.deleteTask(i)); | ||
userListHaveChanges = true; | ||
} catch (NumberFormatException e) { | ||
ui.Speak("need to provide an integer index of task."); | ||
} | ||
|
||
break; | ||
|
||
case "find": | ||
ui.Speak(tasks.findTask(parser.getTaskName())); | ||
break; | ||
|
||
|
||
default: | ||
ui.Speak("OOPS!!! I'm sorry, but I don't know what that means :-("); | ||
|
||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to Java Coding Standard: The switch statement should have the following form: Note there is no indentation for case clauses.
src/main/java/HelperClass/Ui.java
Outdated
|
||
public class Ui { | ||
|
||
private String MyName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try using camelCase for variable names !
src/main/java/HelperClass/Ui.java
Outdated
* print message in a specific format | ||
* @param message the original message to be printed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be good to include an empty line between the description
and @param
!
public void testGetUserCommand() { | ||
Parser parser = new Parser(); | ||
parser.processUserCommand("todo Read Book"); | ||
assertEquals("todo", parser.getCommand()); | ||
} | ||
|
||
@Test | ||
public void testGetTaskName() { | ||
Parser parser = new Parser(); | ||
parser.processUserCommand("todo Read Book"); | ||
assertEquals("Read Book", parser.getTaskName()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to CS2103T Website W3.7d code normally use camelCase for method names e.g., testStringConversion
but when writing test methods, sometimes another convention is used:unitBeingTested_descriptionOfTestInputs_expectedOutcome
.
src/main/java/HelperClass/Task.java
Outdated
* return the string representing how the Task object should be stored | ||
* @return string representation of the Task object | ||
*/ | ||
public String ForRecordingInTextFile() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try using camelCase for method names !
getResponse() only return the user input. Let's update the getResponse() function so it work correctly with duke and returns the right message. In this way, duke GUI can do its job correctly.
When we have a lot of tasks in the list, it would be convenient to have a function that shows the user how many each type of task there are. Let's have a new user command, "stats", to invoke the showTaskStatics() function. It is a convenient way of showing task information to the user.
We need to check some crucial assumptions at certain time during execution of the program. Let's use assert feature to document important assumptions that should hold at various points in the code. In this way, users will know more about where went wrong when using this product.
Add A-Assertions
There are several methods that are too long. Long methods are harder for other developers to understand. Shortening them improves readability of the code. Let's divide long methods into short methods so that other developers can understand the code more easily.
Task names can be empty based on user input. Empty task name is should not be allowed becuase it will lead to bugs. Limiting the task name to be non-empty prevents bugs. Let's limit our user to give only non-empty task names, so that no bugs will occur.
Command functions (todo, deadline, event...) have some common behaviors (return a message). Common behaviors should be abstract out for better OOP practice. Abstracting out common behaviors into a super class and making each command function a separate class allow more conveninent extensions of current commands in the future. Let's have a super class Command and define all commands as child classes of the Command class. Using inheritance is preferable over composition in this situation because the common behaviors are not composable.
Duke-Rio
Duke-Rio helps you store your tasks in a digital list. It's,
All you need to do is,
And it is FREE!
Features:
Here's the main method:
You can modify this line
new Duke("list.txt", "data").run();
in the main function to decide where and what file name is used to store your list.