Skip to content
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

Open
wants to merge 51 commits into
base: master
Choose a base branch
from

Conversation

aang114
Copy link

@aang114 aang114 commented Sep 2, 2019

No description provided.

j-lum and others added 30 commits August 6, 2019 15:25
Add toolVersion block in to Gradle code sample to prevent errors.
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)
Copy link

@tcgj tcgj left a comment

Choose a reason for hiding this comment

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

Looks good!

@Override
public void execute(TaskList taskList, Ui ui, Storage storage) throws IOException {
ui.list();
taskList.list();
Copy link

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();
Copy link

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.

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 {
Copy link

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) {
Copy link

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 {
Copy link

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?

Copy link

@honhaochen honhaochen left a 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) {

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) {

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) {

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants