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

[Seng Zhen Hong] iP #468

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

Conversation

IamZhenHong
Copy link

@IamZhenHong IamZhenHong commented Feb 6, 2024

CharmBot
“Your mind is for having ideas, not holding them.” – David Allen (source)

DukePro frees your mind of having to remember things you need to do. It's,

text-based
easy to learn
FAST SUPER FAST to use
All you need to do is,

add your tasks.
let it manage your tasks for you 😉
And it is FREE!

Features:

Managing tasks
Managing deadlines (coming soon)
Reminders (coming soon)

Copy link

@dhlee03 dhlee03 left a comment

Choose a reason for hiding this comment

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

Nice Code!. Keep working on it:)

while (!command.equals("bye")) {
System.out.println("____________________________________________________________");
try {
if (command.equals("list")) {
Copy link

Choose a reason for hiding this comment

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

The code seems to be too long for me. Try to divide each commands into different functions.

LocalTime endTime = LocalTime.parse(to, DateTimeFormatter.ofPattern("HHmm"));
return new EventTask(description, startTime, endTime);
default:
return new ToDoTask(description);
Copy link

Choose a reason for hiding this comment

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

Consider the case when the file is corrupted


class Task {
protected String description;
protected boolean isDone;
Copy link

Choose a reason for hiding this comment

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

Nice boolean naming👍

}
duke.saveTasksToFile(tasks);

System.out.println("____________________________________________________________");
Copy link

Choose a reason for hiding this comment

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

Maybe consider connecting this three println statements using +.

Copy link

@yorklim yorklim left a comment

Choose a reason for hiding this comment

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

LGTM just some minor nits to consider!

@@ -1,10 +1,283 @@
import java.io.File;
Copy link

Choose a reason for hiding this comment

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

Can consider ordering imports in lexical order

System.out.println("Hello from\n" + logo);
System.out.println("____________________________________________________________");
Copy link

Choose a reason for hiding this comment

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

Can consider making linebreaks into a constant variable which can make code cleaner

String command = scanner.nextLine();
// Initialize tasks list here

while (!command.equals("bye")) {
Copy link

Choose a reason for hiding this comment

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

Can consider breaking down this function into smaller functions to make it more readable for others, might be a little too long currently. Can break down into like two functions like run and commands

scanner.close();
}
} catch (IOException e) {
System.out.println("Error loading tasks from file: " + e.getMessage());
Copy link

Choose a reason for hiding this comment

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

Can consider making this into your own exception class and include the additional error messages as part of the new exception

protected boolean isDone;

public Task(String description) {
this.description = description;
Copy link

Choose a reason for hiding this comment

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

Is there a reason why you added this. infront in task and not in Duke class? Can consider having a constant reference standard.

Copy link

Choose a reason for hiding this comment

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

Good naming of functions and variables and all in all LGTM but just some minor nits

This commit adds an assertion to the parseCommand method in the Parser class to ensure that the array 'parts' has at least one element before further processing. The added assertion enhances the robustness of the code by catching unexpected conditions early in development.

Changes made in this commit:
- Added assertion 'assert parts.length > 0;' to validate the length of the array 'parts'.

This assertion serves as a safeguard against potential runtime errors and improves the overall reliability of the command parsing logic.
Add assertion to ensure non-empty array in parseCommand method
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