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

[Lee Bing Heng] iP #565

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

Conversation

starrylight99
Copy link

@starrylight99 starrylight99 commented Sep 6, 2023

BudgetDuke

  • Cheap clone
  • Lame idea
  • Definitely not for the leyman

How to use

  1. Don't ask me
  2. ?

"Everything LGTM", Sun Tzu probably

public static void main (String[] args) { Duke.die(); }

  • Week 1
  • Week 2
  • Week 3
  • Week 4
  • Week 5

😃
My repo

@@ -20,6 +20,7 @@ enum Type {
DEADLINE("deadline"),
EVENT("event"),
DELETE("delete"),
FIND("find"),

Choose a reason for hiding this comment

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

Great that you have implemented enums to keep track of the long list of keywords!

Comment on lines +208 to +222
protected Response find(String keyword) throws DukeException {
ArrayList<String> output = new ArrayList<>();
output.add("Here are the matching tasks in your list:");

int count = 0;
for (Task task : this.tasks) {
if (task.name().contains(keyword)) {
output.add(String.format("%d.%s", ++count, task.toString()));
}
}
if (count == 0) {
throw new TaskNotFoundException();
}
return Response.generate(output);
}

Choose a reason for hiding this comment

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

Very short and clear code!

Copy link

@suryanshkushwaha suryanshkushwaha left a comment

Choose a reason for hiding this comment

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

LGTM. Overall I feel the code is short and apt.
However, I do feel that it could be better abstracted into more packages.

@@ -0,0 +1,10 @@
package duke;

import duke.Utils.Session;

Choose a reason for hiding this comment

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

are the packages supposed to be in lower case format?

+ ":00";
return LocalDateTime.parse(timeSequence);
}
}

Choose a reason for hiding this comment

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

Great use of Command.java

Comment on lines 1 to 30
package duke.Utils;

/**
* The DukeException class is a custom runtime exception class for handling exceptions
* specific to the Duke application.
* It extends the RuntimeException class and includes a custom error message.
*/
public class DukeException extends RuntimeException {

private String errorMessage;

/**
* Constructs a new DukeException with a custom error message.
*
* @param errorMessage The custom error message to be associated with the exception.
*/
protected DukeException(String errorMessage) {
this.errorMessage = errorMessage;
}

/**
* Returns a string representation of the DukeException, including an error message.
*
* @return A string containing the error message preceded by "OOPS!!!".
*/
@Override
public String toString() {
return "OOPS!!! " + errorMessage;
}
}

Choose a reason for hiding this comment

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

How about having different package just for exceptions?

assertThrows(InvalidArgumentException.class, () -> Command.assertDateTime(string, testCommand));
}
}
}

Choose a reason for hiding this comment

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

Great to see intensive code testing.

Comment on lines +10 to +13
enum Type {
TODO("[T]", 3),
DEADLINE("[D]", 4),
EVENT("[E]", 5);

Choose a reason for hiding this comment

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

Great use of enums here!

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.

4 participants