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

[Wong Zenwei] iP #446

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

Conversation

zeotheburrito
Copy link

@zeotheburrito zeotheburrito commented Feb 3, 2025

[Wong Zenwei] iP Pull Request

Features

  • Show to-do list
  • Add task to to-do list
  • Mark tasks as done

Steps for next increment

  1. Follow the tutorial here
  2. Get the required scripts from text-ui-test in Duke repo
while (true) {
  // keep going!
}

Increments to be done 😭

  • A-TextUiTesting
  • Level-5
  • Level-6
  • A-Enums
  • Level-7
  • Level-8
  • A-MoreOOP
  • A-Packages
  • A-Gradle
  • A-JUnit
  • A-Jar
  • A-JavaDoc
  • A-CodingStandard
  • Level-9
  • A-CheckStyle
  • Level-10
  • A-Varargs

If at first you don't succeed, try, try again

damithc and others added 8 commits July 11, 2024 16:52
In build.gradle, the dependencies on distZip and/or distTar causes
the shadowJar task to generate a second JAR file for which the
mainClass.set("seedu.duke.Duke") does not take effect.
Hence, this additional JAR file cannot be run.
For this product, there is no need to generate a second JAR file
to begin with.

Let's remove this dependency from the build.gradle to prevent the
shadowJar task from generating the extra JAR file.
Copy link

@cyhni cyhni left a comment

Choose a reason for hiding this comment

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

Good job so far, a clean base code to work on going forward.
Some minor coding standard changes requested.

Comment on lines 48 to 80
switch (parsed[0]) {
case "list":
String items = "";
for (int i = 0; i < list.size(); i++) {
items += " " + (i + 1) + ". " + list.get(i) + "\n";
}
message = "____________________________________________________________\n" +
" Here are the tasks in your list:\n" +
items +
"____________________________________________________________\n";
break;
case "mark":
int index = Integer.parseInt(parsed[1]) - 1;
list.get(index).setDone();
message = "____________________________________________________________\n" +
" Nice! I've marked this task as done:\n" +
" " + list.get(index) + "\n" +
"____________________________________________________________\n";
break;
case "unmark":
int ind = Integer.parseInt(parsed[1]) - 1;
list.get(ind).setNotDone();
message = "____________________________________________________________\n" +
" OK, I've marked this task as not done yet:\n" +
" " + list.get(ind) + "\n" +
"____________________________________________________________\n";
break;
default:
list.add(new Task(input));
message = "____________________________________________________________\n" +
" added: " + input + "\n" +
"____________________________________________________________\n";
}
Copy link

Choose a reason for hiding this comment

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

The switch statement should have no indentation for case clauses according to coding standard

Suggested change
switch (parsed[0]) {
case "list":
String items = "";
for (int i = 0; i < list.size(); i++) {
items += " " + (i + 1) + ". " + list.get(i) + "\n";
}
message = "____________________________________________________________\n" +
" Here are the tasks in your list:\n" +
items +
"____________________________________________________________\n";
break;
case "mark":
int index = Integer.parseInt(parsed[1]) - 1;
list.get(index).setDone();
message = "____________________________________________________________\n" +
" Nice! I've marked this task as done:\n" +
" " + list.get(index) + "\n" +
"____________________________________________________________\n";
break;
case "unmark":
int ind = Integer.parseInt(parsed[1]) - 1;
list.get(ind).setNotDone();
message = "____________________________________________________________\n" +
" OK, I've marked this task as not done yet:\n" +
" " + list.get(ind) + "\n" +
"____________________________________________________________\n";
break;
default:
list.add(new Task(input));
message = "____________________________________________________________\n" +
" added: " + input + "\n" +
"____________________________________________________________\n";
}
switch (parsed[0]) {
case "list":
String items = "";
for (int i = 0; i < list.size(); i++) {
items += " " + (i + 1) + ". " + list.get(i) + "\n";
}
message = "____________________________________________________________\n" +
" Here are the tasks in your list:\n" +
items +
"____________________________________________________________\n";
break;
case "mark":
int index = Integer.parseInt(parsed[1]) - 1;
list.get(index).setDone();
message = "____________________________________________________________\n" +
" Nice! I've marked this task as done:\n" +
" " + list.get(index) + "\n" +
"____________________________________________________________\n";
break;
case "unmark":
int ind = Integer.parseInt(parsed[1]) - 1;
list.get(ind).setNotDone();
message = "____________________________________________________________\n" +
" OK, I've marked this task as not done yet:\n" +
" " + list.get(ind) + "\n" +
"____________________________________________________________\n";
break;
default:
list.add(new Task(input));
message = "____________________________________________________________\n" +
" added: " + input + "\n" +
"____________________________________________________________\n";
}

Comment on lines 31 to 34
String greetings = "____________________________________________________________\n" +
" Hello! I'm Jeff\n" +
" What can I do for you?\n" +
"____________________________________________________________\n";
Copy link

Choose a reason for hiding this comment

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

When wrapping lines, break should be before an operator.
I noticed this issues in several other places as well.
Do take note!

Suggested change
String greetings = "____________________________________________________________\n" +
" Hello! I'm Jeff\n" +
" What can I do for you?\n" +
"____________________________________________________________\n";
String greetings = "____________________________________________________________\n"
+ " Hello! I'm Jeff\n"
+ " What can I do for you?\n"
+ "____________________________________________________________\n";

Comment on lines 1 to 3
import java.util.Objects;
import java.util.Scanner;
import java.util.ArrayList;
Copy link

Choose a reason for hiding this comment

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

Perhaps this should be sorted in alphabetical order?

import java.util.ArrayList;

public class Jeff {
public static class Task {
Copy link

Choose a reason for hiding this comment

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

Good intuition to abstract the Task class! 👍

public class Jeff {
public static class Task {
private String desc;
private boolean done;
Copy link

Choose a reason for hiding this comment

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

If I recall correctly, variable names for boolean should be named like a boolean.
e.g use a prefix such as is, has, was

String greetings = "____________________________________________________________\n" +
" Hello! I'm Jeff\n" +
" What can I do for you?\n" +
"____________________________________________________________\n";
Copy link

Choose a reason for hiding this comment

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

Maybe it might be easier in the future to extract the print line to keep as a constant.

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