Skip to content

[Gibson0918] IP#375

Open
Gibson0918 wants to merge 41 commits into
nus-cs2103-AY2223S2:masterfrom
Gibson0918:master
Open

[Gibson0918] IP#375
Gibson0918 wants to merge 41 commits into
nus-cs2103-AY2223S2:masterfrom
Gibson0918:master

Conversation

@Gibson0918
Copy link
Copy Markdown

@Gibson0918 Gibson0918 commented Jan 31, 2023

Duke

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

Duke is a simple reminder app that helps you 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,

  1. download it from here.
  2. double-click it.
  3. add your tasks.
  4. let it manage your tasks for you 😁

And it is FREE!

Features:

  • Adding ToDo task
  • Adding task with deadline
  • Adding Event with deadline

if you are a Java programmer, you can use it to practice Java too. Here's the main method:

    public static void main(String[] args) {
        Application.launch(Main.class, args);
    }
}

@Gibson0918 Gibson0918 changed the title Add Level 1. Greet, Echo, Exit feature IP Jan 31, 2023
@Gibson0918 Gibson0918 changed the title IP [Gibson0918]IP Jan 31, 2023
@Gibson0918 Gibson0918 changed the title [Gibson0918]IP [Gibson0918] IP Jan 31, 2023
Copy link
Copy Markdown

@Guo-KeCheng Guo-KeCheng left a comment

Choose a reason for hiding this comment

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

LGTM. Good job thus far. Jiayous.

Comment thread src/main/java/Duke.java Outdated
Comment on lines +53 to +58
switch (userInput[0]) {
case "bye":
System.out.println("Bye. Hope to see you again soon!");
sc.close();
startDuke = false;
break;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Switch-case should not be indented. Could edit coding style in Intellij to reduce having to manually check.

Comment thread src/main/java/Duke.java Outdated
String[] userInput = sc.nextLine().split(" ", 2);

try {
switch (userInput[0]) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could potentially abstract out userInput[0] using enum

Copy link
Copy Markdown

@sarthak181 sarthak181 left a comment

Choose a reason for hiding this comment

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

Great code overall!

Comment thread src/main/java/Event.java Outdated
Comment on lines +5 to +6
protected LocalDateTime from;
protected LocalDateTime to;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

perhaps startDate and endDate would be better names

Comment thread src/main/java/Event.java Outdated
Comment on lines +15 to +16
return "[E]" + super.toString() + " (from: " + from.format(DateTimeFormatter.ofPattern("MMM dd yyyy HH:mm")) + " to: " + to.format(DateTimeFormatter.ofPattern("MMM dd yyyy HH:mm")) + ")";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this return statement could be indented better

Comment thread src/main/java/duke/command/Command.java Outdated
Comment on lines +10 to +12
public boolean isTerminated() {
return isTerminated;
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

there shouldn't be a semi colon at the end of a method

public class ExitCommand extends Command {

@Override
public void initCommand(TaskList tasks, Ui ui, Storage storage) {
Copy link
Copy Markdown

@rachtan27 rachtan27 Feb 6, 2023

Choose a reason for hiding this comment

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

Good job making the code readable and easy to understand

Comment thread src/main/java/duke/parser/Parser.java Outdated
String[] output = command.split(" ", 2);
ValidCommands validCommands = ValidCommands.valueOf(output[0].toUpperCase());
switch (validCommands) {
case LIST:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remember that there should not be any indentation for case clauses :)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

otherwise, i like how it is very easy to understand what your parser is doing 👍

Comment thread src/main/java/duke/storage/Storage.java Outdated
String[] data = sc.nextLine().split(" \\| ");
Task task = null;
switch (data[0]) {
case "T":
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same issue with the indentation for switch case as mentioned earlier :)

public List<Task> loadFile() throws FileNotFoundException {
List<Task> taskList = new ArrayList<>();
File file = new File(filePath);
if (file.isFile()) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is it possible to avoid this deep nesting?

Gibson0918 and others added 17 commits February 7, 2023 14:08
Remove Java Docs for methods that have been removed.

Refactor Todo, Deadline and Event's toString() and formatForFile() method for better clarity.

Removed unused method in the Ui Class
Tasks already added to Duke cannot be updated so an edit/update feature for existing task would be appreciated as the only way to modify a task would be to delete the existing one and create a new one.

Let's modify the 3 tasks (Event, Todo, Deadline) and add an UpdateCommand class to support Duke in updating existing task
1. Resolve minor GUI bugs where the GUI was not resizing properly

2. Added the missed out set method in TaskList class
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