-
Notifications
You must be signed in to change notification settings - Fork 454
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
[ Hardik Narang ] iP #489
base: master
Are you sure you want to change the base?
[ Hardik Narang ] iP #489
Conversation
This reverts commit 7ea4cc3.
src/main/java/Duke.java
Outdated
public static void readInput(String userInput){ | ||
|
||
// Get command from the userInput | ||
String[] splitLine = userInput.split(" ", 2); |
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.
Shouldn't a plural noun be used for the list of substrings?
src/main/java/Event.java
Outdated
import java.time.format.DateTimeFormatter; | ||
|
||
public class Event extends Task{ | ||
private LocalDateTime at; |
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.
I know its easy to tell from the context, but maybe the variable name can be a little more descriptive? I noticed this in Deadline.java
too 👍
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.
Generally great job for abstraction, few nits on styling to fix for clarity!
data/duke.txt
Outdated
@@ -0,0 +1,8 @@ | |||
T | 1 | new |
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.
Should the save files be uploaded to github?
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.
You should leave duke.txt in the .gitignore file.
src/main/java/Duke.java
Outdated
throw new DoneAlreadyException(); | ||
} | ||
|
||
// Mark the index as done |
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.
Perhaps proper javadocs could be added so IDE can display the documentation with all the information?
src/main/java/Duke.java
Outdated
userInput = userInput.replaceAll( RegexType.START_LINE_REGEX.getRegexType() + command + RegexType.SPACE_REGEX.getRegexType(), ""); | ||
|
||
// Check the command type then execute the commands | ||
if(userInput.equals(InputType.LIST.getInputType())) { |
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.
Could this list of checks better be served with a switch statement to improve readability?
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.
I really liked that you have done a lot of abstraction in your code.
src/main/java/Duke.java
Outdated
import duke.InputType; | ||
import duke.Parser; | ||
import duke.Storage; | ||
import duke.tasks.*; |
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 { | ||
return command.execute(); | ||
} catch (Exception e) { | ||
return new CommandResult(e.getMessage(), false, null); |
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.
Did you not overload your constructor? Do you need to input the "null" argument here?
src/main/java/Duke.java
Outdated
userInput = in.nextLine(); | ||
|
||
// While the input is not "bye", add input to array of tasks and get another input | ||
while (!userInput.equals(InputType.BYE.getInputType())) { |
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.
I think the readibility of this line can be improved by using the "bye" string directly or abstracting this into an ExitCommand as suggested under the specifications. Don't you think so?
if(arguments.equals(InputType.LIST.getInputType())) { | ||
return new ListCommand(taskList); | ||
} |
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.
Doing this kind of defeats the purpose of using enum, right? Instead of using enum purely as strings, I suggest parsing the user input into enum, then using the enum to return the appropriate commands.
An additional suggestion is to use switch case, which is recommended for enum since you can do something like:
if(arguments.equals(InputType.LIST.getInputType())) { | |
return new ListCommand(taskList); | |
} | |
switch (inputType) { | |
case LIST: | |
return new ListCommand(taskList); |
} | ||
return new EmptyCommand(taskList); | ||
} | ||
} |
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.
At the end of every file, you should leave a newline! 😄
src/main/java/Duke.java
Outdated
if (commandResult.isUpdated()) { | ||
taskList = commandResult.getTaskList(); | ||
String errorMessage = storage.writeToFile(taskList); | ||
|
||
ui.printMessage(errorMessage); | ||
} |
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.
I don't think you need to check for this. You can save the file after every command.
import duke.PrintType; | ||
import duke.tasks.TaskManager; | ||
|
||
public class EmptyCommand extends Command{ |
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.
I don't think you need to create a class just to handle an empty command? I suggest just throwing a DukeException for unrecognised strings, since you are using Regex anyways.
* @return - the message of acknowledgement in String form | ||
* @throws TodoException - if the description is empty | ||
*/ | ||
public String addTodo(String userInput) throws TodoException { |
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.
I'm not sure what is the point of having individual command classes where you are doing all the commands under the Task Manager itself?
/** | ||
* Class to maintain the UI of the list of tasks | ||
*/ | ||
public class TaskManagerUi { |
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.
Your UI class should handle interactions with the user? Is this not just a tuple?
*/ | ||
public class Todo extends Task { | ||
private TaskType type = TaskType.TODO; | ||
public Todo (String description){ |
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 to have consistent formatting 😄
public Todo (String description){ | |
public Todo (String description) { |
Ensure the method/class header comments follow the format specified in the coding standard. Points improved: - The Javadoc comment is minimal. - Include tags wherever required and follow the particular order.
Make use of assert feature to document important assumptions that should hold at various points in the code. Refer to this for more details on Java assertions: https://www.geeksforgeeks.org/assertions-in-java/
Merging branch-A-CodeQuality
Add Assertions
Provide a way to attach priorities to tasks such as Low or High This will help the user to prioritize the tasks and become more efficient A task with High priority needs to be done first A task with Low priority needs to be done at the last. Have implemented the extension C-Priority Refer to the other list of extensions here: https://nus-cs2103-ay2122s1.github.io/website/se-book-adapted/projectDuke/index.html#extensions-category-b
Branch c priority
Duke
Duke frees your mind of having to remember things you need to do. It's
EASYSUPER EASY to learnAll you need to do is,
And it is FREE!
Features:
If you Java programmer, you can use it to practice Java too. Here's the
main
method: