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

[clydelhui] iP #373

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

Conversation

clydelhui
Copy link

@clydelhui clydelhui commented Jan 31, 2023

Duke

"How much wood would a woodchuck chuck if a woodchuck could chuck wood? As much as the number of tasks that Duke can store for you." - Clyde Lhui

Duke stores everything for you! It's

  • Fantastic
  • Awesome
  • Written by a cool guy 😸
  • Great THE ABSOLUTE GREATEST

To experience the greatest of Duke,

  1. Download it from here
  2. Click on it
  3. Experience the wonders of Duke!

Features:
[x] Task management
[ ] Search (coming soon)

If you are a Java programmer, you can take a look at the source code too!. Here is the main method in Duke!

public static void main(String[] args) {


        Scanner usrInput = new Scanner(System.in);
        Ui ui = new Ui(usrInput);

        ui.welcome();

        Storage storage = new Storage("data.txt", ui);

        TaskList taskList = new TaskList(storage, ui);

        Parser parser = new Parser();

        String input = "";

        while (true) {
            input = ui.getInput();
            try {
                Command command = parser.parse(input);
                command.execute(taskList, ui, storage);
            } catch (DukeException e) {
                ui.errorDisplay(e);
            }
        }

    }

Copy link

@0x787af25e 0x787af25e left a comment

Choose a reason for hiding this comment

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

Your code is of good quality (though I see some parts that do not conform to the coding standard).

Copy link

@zichen-3974 zichen-3974 left a comment

Choose a reason for hiding this comment

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

Nice job! Your code is clean and easy to follow overall, and your variables are named meaningfully. You may want to consider adding some Javadocs in the future :) 👍

this.taskList = taskList;
}

public void execute(String command) throws IllegalCommandException,

Choose a reason for hiding this comment

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

Perhaps you can consider having Javadocs for this method

public void execute(String command) throws IllegalCommandException,
IllegalInputException, NumberFormatException {
String[] parsedCommand = command.split(" ", 2);
switch (parsedCommand[0]) {

Choose a reason for hiding this comment

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

I like your implementation! Perhaps you can consider further abstraction, e.g having an abstract Command class that different types of commands can extend from :)

super(description, "D");
this.endDate = endDate;
}

public Deadline(String description, boolean done, String endDate) {
public Deadline(String description, boolean done, LocalDate endDate) {

Choose a reason for hiding this comment

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

perhaps the parameter "done" could be changed to "isDone" instead!

Copy link

@JThh JThh left a comment

Choose a reason for hiding this comment

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

The codes are easy to read and well-functional.

}

public void delete(int index){
Task deleted = this.taskList.remove(index - 1);
Copy link

Choose a reason for hiding this comment

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

Can here be some input validity check to ensure index is never less than 1?

@@ -0,0 +1,12 @@
package dukeexceptions;
Copy link

Choose a reason for hiding this comment

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

Please conform to the camelNaming rule here to name the package.

Add additional exceptions with better handling.
# Conflicts:
#	src/main/java/commands/VoidCommand.java
# Conflicts:
#	src/main/java/elems/TaskList.java
When TaskList methods are called, the ArrayList of Tasks should never be
null.

Adding assertions allows us to ensure that this invariant is always
true.

Let's add assertions to the TaskList methods to ensure that all TaskList
Objects which are targets for method invocation have non-null ArrayLists
at the time of method calling.
execute method in the AddCommmand class is long and can be hard to debug
 in the future. If more commands are added, the code will get even
longer.

Let's abstract out the steps taken in each case into a separate method.

Future additions of AddCommands can adopt the same style of execution.
The user may wish to sort the task list by description to make it easy to
read.

Let's add a sorting feature to enable the user to do this.

Some commands such as list and sort will access the list elements. If
the list is empty, they are unable to work as intended.

Let's add an exception to show the case when an empty list is being
accessed by commands that work with the whole list.
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.

5 participants