Skip to content

[Xinran] iP#86

Open
striris wants to merge 32 commits into
nus-cs2113-AY2122S2:masterfrom
striris:master
Open

[Xinran] iP#86
striris wants to merge 32 commits into
nus-cs2113-AY2122S2:masterfrom
striris:master

Conversation

@striris

@striris striris commented Feb 3, 2022

Copy link
Copy Markdown

No description provided.

Add the ability to mark tasks as done. Optionally, add the ability to change the status back to not done.
Add support for tracking three types of tasks:
ToDos: tasks without any date/time attached to it e.g., visit new theme park
Deadlines: tasks that need to be done before a specific date/time e.g., submit report by 11/10/2019 5pm
Events: tasks that start at a specific time and ends at a specific time e.g., team project meeting on 2/10/2019 2-4pm
Comment thread src/main/java/Deadline.java Outdated
}
}

//this.description = description.substring(0, description.lastIndexOf("/") + 4); No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your files should end with a newline!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ok!

Comment thread src/main/java/Duke.java Outdated
System.out.println(bye);
break;
}
else if(userInput.equals("list")){//show 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.

Your comments should be above the line you are commenting

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the reminder! I will make corresponding changes.

Comment thread src/main/java/Duke.java Outdated
for(int i = 0; i<count ; i++)
System.out.println(i+1 + "." + tasks[i]);
}
else if(userInput.split(" ")[0].equals("mark")){//mark task

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

under the coding standards, your else if should be written right next to the bracket above

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I see. Didn't notice that! thanks:)

Comment thread src/main/java/Duke.java Outdated

@chihyingho chihyingho left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good effort! :)

Comment thread src/main/java/Duke.java Outdated
}
else if(input.equals("list")){
else if(userInput.equals("list")){//show list
System.out.println("Here are the tasks in your 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.

Perhaps you could wrap your list function in a method? Additionally, I think the comment can be indented relative to its position in the code (I noticed this in other parts of the code) :)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the reminder! I will make corresponding changes.

Comment thread src/main/java/Duke.java Outdated
System.out.println(i+1 + "." + tasks[i]);
}
else if(input.split(" ")[0].equals("mark")){
else if(userInput.split(" ")[0].equals("mark")){//mark task

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 you can consider using a boolean to reduce the complexity of this if condition? I think this can apply for the other if conditions within this while loop :)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

that's a good idea!

Comment thread src/main/java/Duke.java Outdated
//get the integer k in the string
System.out.println("Nice! I've marked this task as done:");
int k = Integer.parseInt(input.split(" ")[1]);
int k = Integer.parseInt(userInput.split(" ")[1]);

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 you can consider renaming the variable name k with a different name that brings across what k represents?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That has a point:) thanks for the suggestion.

Comment thread src/main/java/Duke.java Outdated

@ElaineQT ElaineQT left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe some parts can be refactored, but generally the code is clear and compact. Good job!

Comment thread src/main/java/Duke.java Outdated
+ "| |_| | |_| | < __/\n"
+ "|____/ \\__,_|_|\\_\\___|\n";
System.out.println("Hello from\n" + logo);
Task[] tasks = new Task[100];

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 the "100" should be set as a constant?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

good idea!

Comment thread src/main/java/Duke.java Outdated
Comment on lines +45 to +51
String toDoDescription = userInput.split("todo")[1].trim();
Task t = new ToDo(toDoDescription);
tasks[count] = t;
count++;
System.out.println("Got it! I've added this task: ");
System.out.println(t);
System.out.println("Now you have " + count + " tasks in the 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.

Would it be better to extract this part as a method? Similarly for adding deadlines and events.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ok! that makes sense.

Comment thread src/main/java/Duke.java Outdated
Comment on lines +61 to +63
System.out.println("Got it! I've added this task: ");
System.out.println(d);
System.out.println("Now you have " + count + " tasks in the 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.

This printing part is used repeatedly. Maybe it can be written as a separate method so that it can be reused.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

sure, I think that would be better also.

Comment thread src/main/java/Duke.java Outdated
Comment on lines +7 to +8
String by = new String();
String at = new String();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These two variables are used only in certain situations. Maybe you can consider adding some utility methods?
References: https://refactoring.guru/smells/temporary-field

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ok I will take a look at this.

@Mick609 Mick609 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Clean code with some naming convention issues.

Comment thread src/main/java/duke/Duke.java Outdated

private static ArrayList<Task> tasks = new ArrayList<>();

private static void greeting() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe you should consider using a verb as the name of a method, like "greet"?

Comment thread src/main/java/duke/Duke.java Outdated
+ "What can I do for you?\n"
+ "----------------------------------------------\n";
String bye = "Bye. Hope to see you again soon!\n";
System.out.println(greeting);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I assume you would want to print the String bye here?

Comment thread src/main/java/duke/Duke.java Outdated
System.out.println(tasks.get(taskIndex-1));
}

private static void addTask(Task 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.

Maybe you can use a more descriptive name for the argument, like "task"?

Comment thread src/main/java/duke/Duke.java Outdated
}

private static void startBot() {
Scanner sc = new Scanner(System.in);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe you can name the Scanner as "scanner"?

Comment thread src/main/java/duke/Duke.java Outdated
}
else if(choice.equals("todo")) {
String toDoDescription = userInput.split("todo")[1].trim();
Task t = new ToDo(toDoDescription);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Maybe you can use a name like "task" for the object?

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.

5 participants