-
Notifications
You must be signed in to change notification settings - Fork 590
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
[Nicholascyx] ip #625
base: master
Are you sure you want to change the base?
[Nicholascyx] ip #625
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.
Looks good.
Requested some cosmetic changes.
src/main/java/Kafka.java
Outdated
} else { | ||
if (userInput[0].equalsIgnoreCase("todo")) { | ||
if (userInput.length < 2) { | ||
throw new KafkaException("It seems you've left the details blank. Even the simplest tasks need some direction, don't you think?"); |
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.
Exception message is quite long and used repeatedly. Maybe store the message as a String variable? Or write a function to throw a new exception?
src/main/java/Event.java
Outdated
@@ -0,0 +1,16 @@ | |||
public class Event extends Task { | |||
|
|||
protected String from; |
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 don't see the protected access modifier often. Quite curious about the reason
src/main/java/Kafka.java
Outdated
__ __ __ _ | ||
| |/ / ____ _/ /_ | | ____ | ||
| / / _ | |_ _| | |/ / / _ | | ||
| \\ | |_| | | | | < | |_| | | ||
|__|\\__\\ \\____| |__| |_|\\ \\ \\____| | ||
"""; |
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.
Nice. I wouldn't be able to create that and adds character to the bot
src/main/java/Kafka.java
Outdated
System.out.println(" Now you have " + this.tasks.size() + " task(s) in the list."); | ||
} | ||
|
||
public void createList() { |
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.
The function name is a little misleading as it does not create a new list. Perhaps getList or printList might be better?
src/main/java/Kafka.java
Outdated
int taskNumber = Integer.parseInt(userInput[1]); | ||
kafka.delete(taskNumber); | ||
} else { | ||
if (userInput[0].equalsIgnoreCase("todo")) { |
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.
Nested if statements are not as easy to read. Is it possible to just add more elif to handle the add task cases instead of nesting them in the else block?
Able to write the list of tasks into Kafka.txt
Easier format to read files
It can now read from the Kafka.txt and update the list.
Exceptions to handle incorrect date format had been added as well.
Some tasklist functions are also added, will continue in the next commit. Created files for Parser and Storage as well
Included some Parser and TaskList functions as well
Finished up all the functions and sync them together correctly
TaskList test is still not done yet, will complete it afterwards
src/main/java/kafka/Storage.java
Outdated
return tasks; | ||
} | ||
|
||
public void printFileContents(ArrayList<Task> tasks) { |
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.
You definitely need JavaDoc comment for this method
src/main/java/kafka/Storage.java
Outdated
} | ||
} | ||
|
||
public void getNewFile(String filePath) { |
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 method should have JavaDoc comment
src/main/java/kafka/Parser.java
Outdated
|
||
public class Parser { | ||
|
||
public static void parseCommand(String userInput, TaskList taskList, Storage storage, Ui ui) throws KafkaException, IOException, DateTimeParseException { |
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.
you need JavaDoc comment for this
src/main/java/kafka/Parser.java
Outdated
case "mark": | ||
int taskNumberMark = Integer.parseInt(arguments); | ||
Task taskToMark = taskList.tasks.get(taskNumberMark - 1); | ||
taskList.mark(taskToMark); | ||
storage.writeToFile(taskList.tasks); | ||
ui.mark(taskToMark); | ||
break; | ||
case "unmark": | ||
int taskNumberUnmark = Integer.parseInt(arguments); | ||
Task taskToUnmark = taskList.tasks.get(taskNumberUnmark - 1); | ||
taskList.unmark(taskToUnmark); | ||
storage.writeToFile(taskList.tasks); | ||
ui.unmark(taskToUnmark); | ||
break; | ||
case "delete": | ||
if (taskList.tasks.isEmpty()) { | ||
return; | ||
} | ||
int taskNumberDelete = Integer.parseInt(arguments); | ||
Task taskToDelete = taskList.tasks.get(taskNumberDelete - 1); | ||
taskList.delete(taskNumberDelete); | ||
storage.writeToFile(taskList.tasks); | ||
ui.delete(taskToDelete, taskList); | ||
break; | ||
case "todo": | ||
if (arguments.isEmpty()) { | ||
throw new KafkaException("It seems you've left the details blank. Even the simplest tasks need some direction, don't you think?"); | ||
} | ||
Task todo = new Todo(arguments, false); | ||
taskList.addTask(todo); | ||
storage.writeToFile(taskList.tasks); | ||
ui.addTask(todo, taskList); | ||
break; | ||
case "deadline": | ||
if (arguments.isEmpty()) { | ||
throw new KafkaException("It seems you've left the details blank. Even the simplest tasks need some direction, don't you think?"); | ||
} | ||
String[] deadlineParts = arguments.split("/by "); | ||
if (deadlineParts.length < 2) { | ||
throw new KafkaException("It appears the details for this deadline task are off. Let's give it another go, shall we?"); | ||
} | ||
LocalDateTime by = LocalDateTimeConverter.getLocalDateTime(deadlineParts[1]); | ||
Task deadline = new Deadline(deadlineParts[0], by, false); | ||
taskList.addTask(deadline); | ||
storage.writeToFile(taskList.tasks); | ||
ui.addTask(deadline, taskList); | ||
break; | ||
case "event": | ||
if (arguments.isEmpty()) { | ||
throw new KafkaException("It seems you've left the details blank. Even the simplest tasks need some direction, don't you think?"); | ||
} | ||
String[] eventParts = arguments.split("/from | /to "); | ||
if (eventParts.length < 3) { | ||
throw new KafkaException("It appears the details for this event task are off. Let's give it another go, shall we?"); | ||
} | ||
LocalDateTime from = LocalDateTimeConverter.getLocalDateTime(eventParts[1]); | ||
LocalDateTime to = LocalDateTimeConverter.getLocalDateTime(eventParts[2]); | ||
Task event = new Event(eventParts[0], from, to, false); | ||
taskList.addTask(event); | ||
storage.writeToFile(taskList.tasks); | ||
ui.addTask(event, taskList); | ||
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.
It is better to add some comments for clarifying
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.
Great Effort! Loved how you customized the different print statements
if (tasks.isEmpty()) { | ||
return; | ||
} | ||
String listMessage = ""; |
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.
Minor nitpick, but you could consider using StringBuilder for some improvement in time complexity. It is really minor though as the strings are probably not long enough to result in any real world performance benefits :)
src/main/java/kafka/Parser.java
Outdated
String command = splitInput[0].toLowerCase(); | ||
String arguments = splitInput.length > 1 ? splitInput[1] : ""; | ||
|
||
switch (command) { |
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.
Wow! This is a really long switch case. Could this be broken up into smaller parts? Maybe try creating a method for each case! This would improve code readability!
import kafka.Deadline; | ||
import kafka.Storage; | ||
import kafka.Todo; | ||
import kafka.Event; | ||
import kafka.Task; | ||
import org.junit.jupiter.api.Test; | ||
import java.io.IOException; | ||
import java.nio.file.Files; | ||
import java.nio.file.Path; | ||
import java.time.LocalDateTime; | ||
import java.util.ArrayList; | ||
import static org.junit.jupiter.api.Assertions.assertEquals; |
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.
Does this follow the package import coding standards?
src/main/java/kafka/Kafka.java
Outdated
} | ||
|
||
public static void main(String[] args) { | ||
new Kafka("C:/Users/Nicholas/Downloads/Kafka.txt").run(); |
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 this work if I run it on a non-windows PC?
public class StorageTest { | ||
|
||
@Test | ||
public void testWriteTodoToFile() throws IOException { |
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 think this might not be the naming convention for testing method. Perhaps can double check it on topics?
src/main/java/kafka/Kafka.java
Outdated
} | ||
|
||
public static void main(String[] args) { | ||
new Kafka("C:/Users/Nicholas/Downloads/Kafka.txt").run(); |
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.
try (FileWriter fw = new FileWriter(filePath)) { | ||
for (Task t : tasks) { | ||
if (t instanceof Todo todo) { | ||
fw.write("T | " + todo.isDone + " | " | ||
+ todo.description + System.lineSeparator()); | ||
} else if (t instanceof Deadline deadline) { | ||
fw.write("D | " + deadline.isDone + " | " | ||
+ deadline.description + " | " + deadline.by + System.lineSeparator()); | ||
} else if (t instanceof Event event) { | ||
fw.write("E | " + event.isDone + " | " | ||
+ event.description + " | " + event.from + " | " + event.to | ||
+ System.lineSeparator()); | ||
} | ||
} |
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'm not quite sure about it (since I couldn't run the file) but I think this method might cause only 1 line of task to be written in the final txt file because the writer object is not being created in append mode. Perhaps it's better to check it out?
# Conflicts: # src/main/java/kafka/Parser.java
Some functions are not yet finished
Fix some issues on the chatbot not printing the list of tasks
Parser method has over 30 lines of code which violates code quality. Let's make each switch case into a different function so that there's only one line of code under every switch case.
Some parts of the code need important assumptions that should hold. Let's add some code using assert function in java to document assumptions.
Some part of my code violates the code quality standards. Let's change them and make it more readable and understandable.
Add assertions
Merge code quality branch
Users sometimes may face the problem of wanting to change the deadline or event date, or delay an event. Let's add a snooze function for users to edit their deadline and event dates.
Made some mistakes in the assertion branch. Let's fix it by adding it again.
Ensures it can be opened across different platforms.
Kafka
Kafka aids you in following the path you are destined to be. It's,
FUNSUPER FUN to useAll you need to do is,
And it is FREE!
Features:
Here's the
main
method: