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.

Sign up for GitHub

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

Jump to bottom

[rohitcube] iP #86

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

[rohitcube] iP #86

wants to merge 45 commits into from

Conversation

rohitcube
Copy link

No description provided.

onx001
onx001 reviewed Sep 7, 2023
View reviewed changes
Copy link

@onx001 onx001 left a comment

Choose a reason for hiding this comment

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

I like the naming conventions used, they were clear and concise. Any reason why you chose to omit javadocs and whitespaces?

src/main/java/Dukey.java Outdated
Comment on lines 9 to 20
String logo = " ____ _ \n"
+ "| _ \\ _ _| | _____ \n"
+ "| | | | | | | |/ / _ \\\n"
+ "| |_| | |_| | < __/\n"
+ "|____/ \\__,_|_|\\_\\___|\n";
String line = "------------------------";

System.out.println(logo);
System.out.println("User");
System.out.println("ToDos: tasks without any date/time attached to it e.g., visit new theme park");
System.out.println("Deadlines: tasks that need to be done before a specific date/time e.g., submit report by 11/10/2019 5pm");
System.out.println("Events: tasks that start at a specific date/time and ends at a specific date/time");
Copy link

Choose a reason for hiding this comment

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

Consider incorporating these lines with the logo to avoid repeating "System.out.println".

src/main/java/Dukey.java Outdated
Comment on lines 42 to 49
String[] parts = userInput.split("/by");
String description = parts[0].substring(9).trim();
String by = parts[1].trim();
tasks.add(new Deadline(description, by));
System.out.println(line);
System.out.println("Got it. I've added this task:");
System.out.println(" " + tasks.get(tasks.size() - 1));
System.out.println("Now you have " + tasks.size() + " tasks in the list.");
Copy link

Choose a reason for hiding this comment

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

Consider adding whitespaces to improve readability.

src/main/java/Dukey.java Outdated
System.out.println(logo);
System.out.println("User");
System.out.println("ToDos: tasks without any date/time attached to it e.g., visit new theme park");
System.out.println("Deadlines: tasks that need to be done before a specific date/time e.g., submit report by 11/10/2019 5pm");
Copy link

Choose a reason for hiding this comment

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

Consider splitting this line into shorter chunks.

src/main/java/Dukey.java Outdated
Comment on lines 71 to 87
} else if (userInput.startsWith("unmark ")) {
int taskIndex = Integer.parseInt(userInput.split(" ")[1]) - 1;
if (taskIndex >= 0 && taskIndex < tasks.size()) {
tasks.get(taskIndex).markAsNotDone();
System.out.println(line);
System.out.println("OK, I've marked this task as not done yet:");
System.out.println(" " + tasks.get(taskIndex));
} else {
System.out.println(line);
System.out.println("Invalid task index.");
}
} else {
tasks.add(new Todo(userInput));
System.out.println(line);
System.out.println("added: " + userInput);
}

Copy link

Choose a reason for hiding this comment

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

Any reason why you chose nested loops?

TriciaBK
TriciaBK reviewed Sep 7, 2023
View reviewed changes
src/main/java/Dukey.java Outdated
Comment on lines 60 to 69
} else if (userInput.startsWith("mark ")) {
int taskIndex = Integer.parseInt(userInput.split(" ")[1]) - 1;
if (taskIndex >= 0 && taskIndex < tasks.size()) {
tasks.get(taskIndex).markAsDone();
System.out.println(line);
System.out.println("Nice! I've marked this task as done:");
System.out.println(" " + tasks.get(taskIndex));
} else {
System.out.println(line);
System.out.println("Invalid task index.");
Copy link

Choose a reason for hiding this comment

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

Try to avoid arrowhead style convention. Perhaps you could have the marking and unmarking of task as a separate method.

src/main/java/Dukey.java Outdated
}
}

class Task {
Copy link

Choose a reason for hiding this comment

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

You should probably store the different classes in different files

src/main/java/Dukey.java Outdated
import java.util.Scanner;

public class Dukey {
public static void main(String[] args) {
Copy link

Choose a reason for hiding this comment

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

Avoid long methods - could consider having different methods outside of the main method

YongbinWang
YongbinWang reviewed Sep 16, 2023
View reviewed changes
Copy link

@YongbinWang YongbinWang left a comment

Choose a reason for hiding this comment

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

Overall good job so far. Some minor coding standard nitpicks from me. Good naming of variables and code quality. Keep up the good work!

src/main/java/Dukey.java Outdated
Comment on lines 3 to 4
import Tasks.*;
import dukey.*;
Copy link

Choose a reason for hiding this comment

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

Imported classes should always be listed explicitly.

src/main/java/Dukey.java Outdated
tasks.add(new Todo(description));
tasks.get(tasks.size() - 1).printNewTask();
} catch(IndexOutOfBoundsException e) {
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.

Consider abstracting this out into a constant variable called LINE_DIVIDER.

src/main/java/Dukey.java
public static void main(String[] args) {
System.out.println("Hey! I'm Dukey, your virtual assistant!\nWhat can I do for you?\n");
Scanner in = new Scanner(System.in);
String line;
Copy link

Choose a reason for hiding this comment

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

Consider changing the variable name to something more appropriate such as userInput.

src/main/java/Dukey.java Outdated
Comment on lines 87 to 88
switch (firstWord) {
case "bye":
Copy link

Choose a reason for hiding this comment

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

Take note of the coding standard violation here, the case clause should not have an indentation. You can change your IDE to follow the coding standard.

rohitcube and others added 19 commits September 21, 2023 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onx001 onx001 onx001 left review comments

@YongbinWang YongbinWang YongbinWang left review comments

@TriciaBK TriciaBK TriciaBK left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rohitcube @onx001 @YongbinWang @TriciaBK