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

[Ming Liang Ng] Duke Increments #19

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

Conversation

Dban1
Copy link

@Dban1 Dban1 commented Jan 24, 2020

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
The OpenJFX plugin expects applications to be modular and bundled
with jlink, resulting in fat jars that are not cross-platform. Let's
manually include the required dependencies so that shadow can package
them properly.
-added error handling for unknown commands
-added error handling for wrong number of parameters given for commands
-added error handling for "done" if number is not given in parameter [1]
-added "delete" command along with error handling similar to "done"
-Improved checkstyle configuration
…meant for conversion into string for storage

-Duke.java now reads/stores from the ./../data/duke.txt file
-created the Parser, Storage, TaskList and Ui class
-To-dos (Parser.java)
-created the Parser, Storage, TaskList and Ui class
-To-dos (Parser.java)
-other touch ups to get OOP Duke functional
Copy link

@JeremyLoh JeremyLoh left a comment

Choose a reason for hiding this comment

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

Good to see OOP used for your Duke. Consider refactoring out your Command class (e.g. DeleteCommand, parsing functions to be placed in Parser class instead).
Good job! 👍

@@ -0,0 +1,162 @@
package duke.commands;

import duke.tasks.*;

Choose a reason for hiding this comment

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

Try to import all classes used, instead of *.
In IntelliJ, the * import can be disabled by going to
Settings -> Editor -> Code Style -> Java, and under the Imports tab, the number of class count before * is used can be increased. (Under Class count to use import with *). I've changed mine to a reasonably higher number (200).

image

}

public void execute(TaskList taskList, Storage storage, Ui ui) throws DukeException {
switch (this.commandWord) {

Choose a reason for hiding this comment

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

Good use of the switch statement, instead of having if else blocks and using equals()
If you like to refactor this Command class, you could break up the commands into subclasses (e.g. DeleteCommand, ListCommand etc.)

Choose a reason for hiding this comment

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

Yes, I think you could place Commands enum in a separate Java file, for better code readability.

} catch (Exception e) {
throw new DukeException("Input a number boi.");
}
if (Integer.parseInt(this.elements[1]) > taskList.getTaskList().size()) {

Choose a reason for hiding this comment

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

Good to see error checking for the done command 👍

this.isExit = true;
break;
case "list":
if (taskList.getTaskList().size() < 1) {

Choose a reason for hiding this comment

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

Maybe the task list and size of the task list could be placed outside of the switch statement as variables, to prevent extra calls to the getTaskList() and size() function

import java.util.List;
import java.util.ArrayList;

import duke.tasks.*;

Choose a reason for hiding this comment

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

import duke.tasks.* should be replaced with the individual classes in the tasks package.

Comment on lines 26 to 31
/**
* Builds a TaskList class from storage Filepath.
* @return Returns a TaskList class built according to storage Filepath. Returns an empty TaskList class
* if there is no storage file.
* @throws DukeException Filepath not found.
*/

Choose a reason for hiding this comment

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

Based on the coding standards here, there should be an empty line between description and parameter section.

}

/**
* Saves given TaskList to the storage file.

Choose a reason for hiding this comment

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

Slight formatting problem for JavaDoc: require empty line between parameters and description.

Comment on lines 23 to 24
* Creates a Deadline class.
* @param strArr Array of String containing input for the Deadline class.

Choose a reason for hiding this comment

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

Needs a blank line between the JavaDoc description and parameter.

Comment on lines +5 to +9
public class Parser {
public static Command parseCommand(String input) {
return new Command(input);
}
}

Choose a reason for hiding this comment

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

Maybe you could place the functions related to parsing user string input here instead of putting it inside the Command class.

*/
public void farewellUser() {
divider("Aww, see you again soon!");
return;

Choose a reason for hiding this comment

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

I think the return; statement here could be removed as this function has a return type of void.

Copy link

@nathanaelseen nathanaelseen left a comment

Choose a reason for hiding this comment

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

Good OOP design overall, the entities are logically and appropriately modularised.


/**
* Constructs a Duke class.
* @param filePath Filepath of the storage file tasks.txt

Choose a reason for hiding this comment

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

Try to leave an empty line between method description parameter section.


/**
* Creates an Event class.
* @param strArr Array of String containing input for the Event class.

Choose a reason for hiding this comment

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

Try to leave and empty line between parameter and method descriptions.


/**
* Prints out input String within two divider lines. If input string is empty, prints out single line.
* @param s String to be wrapped by two divider lines.

Choose a reason for hiding this comment

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

Try to leave an empty line between parameter description and method description.

}

public void execute(TaskList taskList, Storage storage, Ui ui) throws DukeException {
switch (this.commandWord) {

Choose a reason for hiding this comment

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

Yes, I think you could place Commands enum in a separate Java file, for better code readability.

@jothipillay
Copy link

image

Try not to use magic numbers in your code! Declare values inside static final variables and use them instead. Other than that everything generally looks great. Keep up the good work! :)

…dity of dates before current date

-shortened `divider` method in Ui
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.

6 participants