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

[Jade Tay] Duke Increments #142

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

Conversation

jadetayy
Copy link

Functionalities added:

  1. Added tasks (Todo, Events, Deadlines)
  2. Completing tasks
  3. Removing tasks
  4. Displaying tasks

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

@alcen alcen left a comment

Choose a reason for hiding this comment

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

Generally pretty clear code, with almost all methods having Javadoc comments! Great!!

I have left some comments below for further discussion, especially about the DukeException class. Hope you find them useful! Do reply if you have any concerns about them

setAlignment(Pos.TOP_LEFT);
}

public static DialogBox getUserDialog(String text, Image img) {
Copy link

Choose a reason for hiding this comment

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

I think it's a good idea to have Javadoc comments here, for a public method

return new DialogBox(text, img);
}

public static DialogBox getDukeDialog(String text, Image img) {
Copy link

Choose a reason for hiding this comment

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

Here as well, would it be clearer if we put a Javadoc comment here? Seems more useful than the existing comment for the private flip method

/**
* Creates a new Exception when thrown.
*/
public class DukeException extends Exception {
Copy link

Choose a reason for hiding this comment

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

To me, the name DukeException seems quite ambiguous. Granted, it seems like all Exception files in Duke extend from DukeException. Interesting way to structure the Exceptions, but I'm not sure of the difference if each individual Exception extended directly from Exception instead

src/main/java/Duke.java Outdated Show resolved Hide resolved
src/main/java/Duke.java Outdated Show resolved Hide resolved
src/main/java/task/Task.java Show resolved Hide resolved
/**
* This class contains the task list.
*/
public class TaskList {
Copy link

Choose a reason for hiding this comment

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

I feel that this class does not do much that an ArrayList<Task> cannot do, so I was wondering whether this class serves any other purpose that I might have missed?

src/main/java/ui/Ui.java Outdated Show resolved Hide resolved
src/test/java/task/DeadlineTest.java Outdated Show resolved Hide resolved
test-ui-test/ACTUAL.TXT Show resolved Hide resolved
data/tasks.txt Outdated Show resolved Hide resolved
/**
* Constructor for AddCommand.
*
* @param t t is the Task to be added into TaskList.
Copy link

Choose a reason for hiding this comment

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

You can have a better variable naming as well as a more intuitive javadoc

storage.saveTasks(tasks.getTasks());
String msg = "Got it! I've added this task!: \n";
msg += " " + this.task;
msg += "\nNow you have " + tasks.getNumTasks() + " tasks in the list.";
Copy link

Choose a reason for hiding this comment

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

You can apply SLAP here. You can abstract out the string concatenation

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