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

[Guo Fengming] ip #486

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

Conversation

Gu0Fengming
Copy link

No description provided.

damithc and others added 13 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

@VikramGoyal23 VikramGoyal23 left a comment

Choose a reason for hiding this comment

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

There seems to be several small coding standard violations, but beyond that I only have a couple of nitpicks.

@@ -0,0 +1,168 @@
import java.io.BufferedReader;

Choose a reason for hiding this comment

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

You should put this class in a package, maybe main or whiost.

Comment on lines 29 to 40
String greeting = "Hello! I'm Whiost\nWhat can I do for you?\n";
String addTask = "Got it. I've added this task:";
String marked = "Nice! I've marked this task as done:";
String unmarked = "OK, I've marked this task as not done yet:";
String reportTask1 = "Now you have ";
String reportTask2 = " tasks in the list.";
String deleted = "Noted. I've removed this task:";

String error0 = "OOPS!!! I'm sorry, but I don't know what that means.";
String errorEmpty = "OOPS!!! The description cannot be empty.";
String errorNotFound = "OOPS!!! The task you select doesn't exist.";
String errorEmptyList = "OOPS!!! There's no task.";

Choose a reason for hiding this comment

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

Since all of these strings are never reassigned, you should consider writing the variable names in SCREAMING_SNAKE_CASE to indicate they are constants.

String reportTask2 = " tasks in the list.";
String deleted = "Noted. I've removed this task:";

String error0 = "OOPS!!! I'm sorry, but I don't know what that means.";

Choose a reason for hiding this comment

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

error0 is a quite undescriptive variable name, maybe errorUnknown or errorInvalidInput could be better.

// Print greeting
System.out.print(greeting);

boolean p = true;

Choose a reason for hiding this comment

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

p is a very undescriptive variable name, maybe something like isInputting could be better (it should at least sound like a boolean variable).


try {
BufferedReader data = new BufferedReader(new FileReader("./data/whiost.txt"));
List<String> temp_lst = new ArrayList<String>();

Choose a reason for hiding this comment

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

Variables should not be written in snake_case, you should change the variable name to tempLst.

import java.util.List;

public class Whiost {
public static void save(List<String> lst, List<String> typeLst, List<String> markLst) {

Choose a reason for hiding this comment

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

Consider adding a JavaDoc header for this method to explain what it does..

import java.util.ArrayList;
import java.util.List;

public class Whiost {

Choose a reason for hiding this comment

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

Consider adding a JavaDoc header for the class to explain what it does.

Choose a reason for hiding this comment

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

Try and use more abstraction (e.g. separate classes) for the different types of tasks the bot will keep track of.

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