Skip to content

[bljhty] iP#65

Open
bljhty wants to merge 36 commits into
nus-cs2113-AY2324S1:masterfrom
bljhty:master
Open

[bljhty] iP#65
bljhty wants to merge 36 commits into
nus-cs2113-AY2324S1:masterfrom
bljhty:master

Conversation

@bljhty

@bljhty bljhty commented Sep 4, 2023

Copy link
Copy Markdown

No description provided.

@SebasFok SebasFok 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.

Looks good! Very clear comments used to allow readers to understand your train of thought

Comment thread src/main/java/Duke.java Outdated
+ " | |_| | (_) | | | | | |\n"
+ " |____/ \\___/|_| |_| |_|\n";
System.out.println("Hello from\n" + logo);
String greetings = "____________________________________________________________" + "\n" +

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 the line be stored in a string variable so that it could be used more easily elsewhere when formatting?

Comment thread src/main/java/Duke.java Outdated
* This method prints the list of tasks.
*/
private static void listTasks(ArrayList<Task> tasks) {
if (tasks.isEmpty()) {

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 like how you considered the case when the list is empty so that the user knows that his input was read.

Comment thread src/main/java/Duke.java Outdated
Comment on lines +50 to +61
Task task = tasks.get(i);
String taskType = task instanceof Todo ? "[T]" : (task instanceof Deadline ? "[D]" : "[E]");
String statusIcon = task.getStatusIcon();
String description = task.getDescription();

if (task instanceof Deadline) {
description += " (by: " + ((Deadline) task).getBy() + ")";
} else if (task instanceof Event) {
description += " (from: " + ((Event) task).getFrom() + " to: " + ((Event) task).getTo() + ")";
}

System.out.println(" " + (i + 1) + "." + taskType + statusIcon + " " + description);

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 like how you broke the sentence up into multiple parts such that it is easy to modify it in the future. There is also good use of white spaces to make make the sentence clear to the reader.

Comment thread src/main/java/Duke.java Outdated
String command = givenTask.nextLine();
System.out.println("____________________________________________________________");

if (command.equalsIgnoreCase("bye")) {

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 the if else statements be changed to switch statements for better readability? This could make each branch easier to find and edit in the future.

Bryan Lee added 10 commits September 13, 2023 12:45
* Branch-Level-5:
  Level-5 increment + edit code
  edited code

# Conflicts:
#	src/main/java/Duke.java
This reverts commit f50b1a3.
* branch-A-package:
  included packages

@irving11119 irving11119 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.

Overall, your code follows the coding standards and your naming conventions are good. However, do try to avoid deep nested as it affects readability of code and try to use constants rather than magic strings and numbers!

@@ -0,0 +1 @@
/Users/bryanlee/Desktop/NUS/NUS Y3S1/CS2113/ip/src/main/java/Duke.java 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.

Perhaps you could add this file to your .gitignore.

Comment thread src/main/java/Duke.java Outdated
System.out.println(goodbye);
}

public static void getHelp() {

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 consider making this a named constant instead to avoid having magic strings.

Comment thread src/main/java/Duke.java Outdated
Comment on lines +119 to +140
} else if (command.startsWith("mark")) {
if (command.substring(5).isEmpty()) {
System.out.println("Error: Task number cannot be empty\n");
System.out.println("Please enter command in the format: mark <task number>");
}
try {
String[] parts = command.split(" ");
if (parts.length == 2) {
int taskIndex = Integer.parseInt(parts[1]) - 1;
if (taskIndex >= 0 && taskIndex < tasks.size()) {
Task task = tasks.get(taskIndex);
task.markAsDone(true);
System.out.println(
" Nice! I've marked this task as done:\n" + " " + task.getStatusIcon()
+ " " + task.getDescription());
} else {
System.out.println(" Invalid task number.");
}
}
} catch (NumberFormatException e) {
System.out.println("Please enter a valid task number");
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Try to avoid deep nesting here and elsewhere in your code. Instead you could try to implement some of the nested logic in another method and call the method!

Bryan Lee and others added 11 commits September 19, 2023 23:19
change from else if statements to case statements
* branch-level-6:
  Level-6 increment done
* branch-level-7:
  added level-7 increments

# Conflicts:
#	src/main/java/Duke.java
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.

3 participants