-
Notifications
You must be signed in to change notification settings - Fork 429
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
# Chatty #445
base: master
Are you sure you want to change the base?
# Chatty #445
Conversation
In build.gradle, the dependencies on distZip and/or distTar causes the shadowJar task to generate a second JAR file for which the mainClass.set("seedu.duke.Duke") does not take effect. Hence, this additional JAR file cannot be run. For this product, there is no need to generate a second JAR file to begin with. Let's remove this dependency from the build.gradle to prevent the shadowJar task from generating the extra JAR file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how many potential user errors are considered and accounted for. Perhaps the code could be cleaner by refactoring?
src/main/java/Chatty.java
Outdated
System.out.println("What can I do for you?"); | ||
ArrayList<Task> userInputs = new ArrayList<Task>(); | ||
try { | ||
String s = br.readLine(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this have a more descriptive variable name? such as userInput
src/main/java/Chatty.java
Outdated
String s = br.readLine(); | ||
while (s != null) { | ||
System.out.println("____________________________________________________________"); | ||
if (s.equals("bye") || s.equals("Bye")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would using s.equalsIgnoreCase("bye")
here be more efficient and readable?
src/main/java/Chatty.java
Outdated
Pattern p = Pattern.compile("([0-9]+$)"); | ||
Matcher m = p.matcher(s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this two line definition is used multiple times instead of using a helper method?
(for example again in line 52-53)
src/main/java/Chatty.java
Outdated
} | ||
} else { | ||
System.out.println("Invalid item"); | ||
} | ||
} else { | ||
System.out.println("Invalid item"); | ||
} | ||
} else if (s.contains("delete")) { | ||
Pattern p = Pattern.compile("([0-9]+$)"); | ||
Matcher m = p.matcher(s); | ||
if (m.find()) { | ||
int i = Integer.parseInt(m.group(1)) - 1; | ||
if (i < userInputs.size()) { | ||
Task task = userInputs.get(i); | ||
System.out.println("Noted. I've removed this task:"); | ||
System.out.println(" " + task); | ||
userInputs.remove(i); | ||
System.out.println("Now you have " + userInputs.size() + " items in the list."); | ||
} else { | ||
System.out.println("Invalid item"); | ||
} | ||
} else { | ||
System.out.println("No such item"); | ||
} | ||
|
||
} else { | ||
if (s.contains("todo")) { | ||
Pattern titlePattern = Pattern.compile("todo\\s*(.*)"); | ||
Matcher titlePatternMatcher = titlePattern.matcher(s); | ||
String todoTitle = ""; | ||
if (titlePatternMatcher.find()) { | ||
todoTitle = titlePatternMatcher.group(1); | ||
} else { | ||
System.out.println("Invalid title"); | ||
} | ||
|
||
Todo t = new Todo(todoTitle); | ||
userInputs.add(t); | ||
System.out.println("Got it. I've added this task:\n " + t); | ||
System.out.println("Now you have " + userInputs.size() + " items in the list."); | ||
} else if (s.contains("event")) { | ||
Pattern titlePattern = Pattern.compile("event\s*(.*?)\s+/"); | ||
Matcher titlePatternMatcher = titlePattern.matcher(s); | ||
String eventTitle = ""; | ||
if (titlePatternMatcher.find()) { | ||
eventTitle = titlePatternMatcher.group(1); | ||
} else { | ||
System.out.println("Invalid title"); | ||
} | ||
|
||
Pattern fromPattern = Pattern.compile("/from\s*(.*?)\s+/"); | ||
Matcher fromPatternMatcher = fromPattern.matcher(s); | ||
String fromString = ""; | ||
if (fromPatternMatcher.find()) { | ||
fromString = fromPatternMatcher.group(1); | ||
} else { | ||
System.out.println("Invalid from timing"); | ||
} | ||
|
||
Pattern toPattern = Pattern.compile("/to\\s*(.*)"); | ||
Matcher toPatternMatcher = toPattern.matcher(s); | ||
String toString = ""; | ||
if (toPatternMatcher.find()) { | ||
toString = toPatternMatcher.group(1); | ||
} else { | ||
System.out.println("Invalid to timing"); | ||
} | ||
|
||
Event e = new Event(eventTitle, fromString, toString); | ||
userInputs.add(e); | ||
System.out.println("Got it. I've added this task:\n " + e); | ||
System.out.println("Now you have " + userInputs.size() + " items in the list."); | ||
} else if (s.contains("deadline")) { | ||
Pattern titlePattern = Pattern.compile("deadline\s*(.*?)\s+/"); | ||
Matcher titlePatternMatcher = titlePattern.matcher(s); | ||
String deadlineTitle = ""; | ||
if (titlePatternMatcher.find()) { | ||
deadlineTitle = titlePatternMatcher.group(1); | ||
} else { | ||
System.out.println("Invalid title"); | ||
} | ||
|
||
Pattern byPattern = Pattern.compile("/by\\s*(.*)"); | ||
Matcher byPatternMatcher = byPattern.matcher(s); | ||
String byString = ""; | ||
if (byPatternMatcher.find()) { | ||
byString = byPatternMatcher.group(1); | ||
} else { | ||
System.out.println("Invalid by timing"); | ||
} | ||
|
||
Deadline d = new Deadline(deadlineTitle, byString); | ||
userInputs.add(d); | ||
System.out.println("Got it. I've added this task:\n " + d); | ||
System.out.println("Now you have " + userInputs.size() + " items in the list."); | ||
|
||
} else { | ||
System.out.println("Invalid item"); | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should switch-case be used instead of this many if-else statements to increase readability and maintainability?
src/main/java/Chatty.java
Outdated
System.out.println(" " + task); | ||
} | ||
} else { | ||
System.out.println("Invalid item"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For error messages, should there be logics or other ways to implement them so that they are not hardcoded?
src/main/java/Chatty.java
Outdated
|
||
} else { | ||
System.out.println("Invalid item"); | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may break the app as it breaks out of the while loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this code be refactored to improve readability and logic so that there are less hardcoding involved?
# Conflicts: # src/main/java/running/TaskList.java # src/main/java/tasks/Task.java
Branch-A-Assertions
Parser execute method currently has much more than 50 LoC and has too many levels/layers of nesting Add new methods to Parser class in order to increase level of abstraction and also reduce length of execute method Add recur functionality to create repeated tasks at daily/weekly/monthly/yearly intervals for a specified number of repetitions
Display list and mark functions
Branch-A-CodeQuality
Chatty
Chatty helps you keep track of the million things on your to-do list, but it's also
All you need to do is,
Here's what an example task list looks like:
If you are a Java programmer, you can use it to practice Java too. Here's the main method:
TRY IT OUT NOW!