-
Notifications
You must be signed in to change notification settings - Fork 315
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
[Karan Dev Sapra] Duke Increments #356
base: master
Are you sure you want to change the base?
Conversation
Add toolVersion block in to Gradle code sample to prevent errors.
Change file mode on `gradle` to be executable (nus-cs2103-AY1920S1#9)
Gradle defaults to an empty stdin which results in runtime exceptions when attempting to read from `System.in`. Let's add some sensible defaults for students who may still need to work with the standard input stream.
Add configuration for console applications
… does not follow the dd/MM/yyyy... format)
… & Wrapped Single statement conditionals & Single statement classes with curly brackets)
merging gradle into master
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.
Looks good!
src/main/java/ListCommand.java
Outdated
@Override | ||
public void execute(TaskList taskList, Ui ui, Storage storage) throws IOException { | ||
ui.list(); | ||
taskList.list(); |
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 you might want to make the two methods here somewhat clearer in terms of naming? Both ui.list()
and taskList.list()
seem to mean the same thing even though they most likely do very different things.
case "bye": | ||
return Bot.TaskType.BYE; | ||
return new ExitCommand(); |
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.
Would it be better if you kept these two commands together with the rest of the switch statement so that it remains as a block? If you have no spaces, split actually returns you an array containing just the command itself, so word[0]
would still correspond to "list" and "bye" in those cases.
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.
Yes, I do agree with Terence
|
||
} | ||
|
||
public static Task parsesAdd (String command, TaskType taskType) throws DukeException { |
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 it may be better to rename your function to an imperative, maybe parseAdd
? This is so that the function adheres to coding standards, and states "what it will do" rather than "what it does".
|
||
String description; | ||
|
||
switch (taskType) { |
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.
Hmm, I wonder if it's very efficient if you have another switch statement here considering you already individually parsed the type of task to add inside of your parse
function. Not sure if it would be better to have three different functions here instead. Might be debatable!
FileWriter fw = new FileWriter(filePath); | ||
fw.write(textToAdd); | ||
fw.close(); | ||
} | ||
|
||
public void retrieve () throws FileNotFoundException { | ||
File f = new File(Storage.DUKE_TXT_PATH); // create a File for the given file path | ||
public ArrayList<Task> retrieve () throws FileNotFoundException { |
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.
Would it be better if you checked if the file existed, then returned an empty list, rather than throw a FileNotFoundException
if it didn't already exist?
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.
Command Class:
- Under execute method, i think it's very expensive to store the entire list again and again for every command. Wouldn't it already served the purpose if you just save it in during your ExitCommand?
Overall:
- Some files are missing JavaDocs (FindCommand)
- Great coding conventions
- Great implementations! However, some recommendations can be found in the code specifically.
|
||
try { | ||
date = format.parse(string); | ||
} catch (ParseException e) { |
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.
Would it be better to handle the exception instead of doing nothing?
Let's say you can throw ParseException to Duke class and let it handle the output/ message.
try { | ||
tasks = new TaskList(storage.retrieve()); | ||
} catch (FileNotFoundException e) { | ||
|
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.
Would it be better to inform the user about the exception?
For me particularly I would inform that "file isn't found, a new file will be created instead"
return task; | ||
} | ||
|
||
public Task[] find(String text) { |
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.
Would it be easier to return ArrayList?
…t all the text fits as the TaskList shrinks or grows
…ld at various points in the code.
…ality (where necessary)
Critically examined the code and refactored it to improve the code qu…
Added assert feature to document important assumptions that should ho…
No description provided.