-
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
[audipras] iP #629
base: master
Are you sure you want to change the base?
[audipras] iP #629
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.
This reverts commit 66ca9d2.
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.
In summary, your code largely follows good naming conventions, which significantly contributes to its readability and maintainability. The main areas for improvement are in using more descriptive class names and avoiding abbreviations like descrip. These changes will make your code even more intuitive and easier for others to understand.
Keep up the good work! :))
src/main/java/Parser.java
Outdated
@@ -0,0 +1,80 @@ | |||
import task.TaskList; |
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.
Parser is a good name for a class responsible for parsing input. It clearly describes its purpose. Well done here!
src/main/java/Maga.java
Outdated
private void run() { | ||
printGreeting(); | ||
TaskManager taskManager = new TaskManager(); | ||
TaskList taskList = initialiseBot(taskManager); |
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.
accurately describes the action it performs, which is good practice.
src/main/java/task/TaskList.java
Outdated
} | ||
|
||
public void deleteTask(int taskNumber) { | ||
Task tempTask = taskList[taskNumber]; |
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.
tempTask is used to refer to a task temporarily during deletion or other operations, very well used!
src/main/java/task/TaskList.java
Outdated
} | ||
} | ||
|
||
public void addTask(String input) { |
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 variable input is well-named as it clearly indicates that it holds user input. It is appropriately concise and descriptive.
src/main/java/task/TaskList.java
Outdated
public void addTask(String input) { | ||
Task task = new TodoTask(""); | ||
if(input.startsWith("todo ")) { | ||
String descrip = input.substring(5).trim(); |
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 abbreviation descrip is a bit unclear at first glance. It would be better to use the full word description for clarity.
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.
Overall, LGTM. Your OOP principles are generally well followed in your code. I feel the readability of your code could be enhanced by adding in Javadoc comments, and also perhaps using enums. Some other nits to fix here and there.
src/main/java/Maga.java
Outdated
import task.TaskList; | ||
import task.TaskManager; | ||
|
||
public class Maga { |
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 your use of OOP concepts to make your main class clean and easy to read
src/main/java/Parser.java
Outdated
continue; | ||
} | ||
|
||
// mark tasks as done |
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.
comments to explain each command follow coding style well and helpful in understanding the code
src/main/java/Parser.java
Outdated
} | ||
|
||
public void handleInput(String input) { | ||
while(!input.equalsIgnoreCase("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.
You could consider the use of enums instead of representing Tasks as Strings.
src/main/java/Maga.java
Outdated
private TaskList initialiseBot(TaskManager taskManager) { | ||
return taskManager.loadTasks(); | ||
} | ||
|
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 you are missing Javadoc commands across your codebase? I understand it is time consuming, but perhaps writing javadocs for at least the key methods will enable future readers of your code to understand it more seamlessly.
src/main/java/task/Task.java
Outdated
String to = parts[4]; | ||
return new DeadlineTask(isDone, description, from, to); | ||
} | ||
|
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.
Did you mean to have 'from' and 'to' parameters for Deadline and 'dateTime' for Event? Logically speaking, they should be the other way around don't you think?
src/main/java/task/TaskManager.java
Outdated
public TaskList loadTasks() { | ||
TaskList tasks = new TaskList(100); | ||
File file = new File(FILE_PATH); | ||
|
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 that you are sticking to the style when it comes to indenting comments relative to their position in the code
Add Command, Controller and InvalidCommandException classes Add Junit Tests for delete, mark and unmark functions
Using the exit command now properly closes javafx window after a pause. Bot now properly saves tasks to the task file after every change to the task list.
Create packages 'ui', 'middleware', 'commands' and 'exceptions' to further organise code.
Add new exception
Add assertions
Tagging tasks is unavailable. User can't categorise tasks since description can't be changed. Add new 'tag' string field to each task, and add new 'tag' command so users can tag current tasks.
No help manual for the user. Create user guide to orient the user on how to use the bot. The user can immediately use the bot without facing errors with bot usage.
MAGA Chatbot
The release of this chatbot is yuuuuuge - like, YUUUGE. It's gonna solve ALL your task-related problems. Just trust me - its the best 👌. It has:
TRUMPDRUMPF SPEECHFeatures:
People who endorse this app:
I support all jobs and occupations in America - yes, even our dear musicians and artists. Look at the beautiful art this bot has:
Download this bot nowww 👌 - Make America Great Again!!