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

[Tan Xuan Zhi] iP #344

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

[Tan Xuan Zhi] iP #344

wants to merge 49 commits into from

Conversation

xxzz-tt
Copy link

@xxzz-tt xxzz-tt commented Aug 25, 2020

No description provided.

Copy link

@tengjianling tengjianling left a comment

Choose a reason for hiding this comment

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

Pretty good job except that some methods can be split into different methods to become shorter. Good job!

@@ -0,0 +1,10 @@

enum Chat {

Choose a reason for hiding this comment

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

Should the enum instances be fully capitalized?

Copy link

Choose a reason for hiding this comment

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

Enums are constants so perhaps it is fine to be fully capitalised.

@@ -0,0 +1,145 @@
import java.io.*;

Choose a reason for hiding this comment

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

Maybe list out the imports explicitly instead of using .*?

Comment on lines 2 to 11
import java.util.ArrayList;
import java.util.List;
import java.util.Scanner;
import java.time.LocalDateTime;
import java.time.format.FormatStyle;
import java.util.ArrayList;
import java.util.List;
import java.time.LocalDate;
import java.time.format.DateTimeFormatter;
import java.time.temporal.ChronoUnit;

Choose a reason for hiding this comment

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

Maybe arrange the imports in lexicology order?

System.out.println(line);
System.out.println("Got it. I've added this task:");
System.out.println(t.printTask());
System.out.println("Now you have " + list.size() + (list.size() > 1 ? " tasks" : " task") + " on the list");

Choose a reason for hiding this comment

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

Maybe split this line into different lines for better readability?

case "bye":
exit();
return false;
// break;

Choose a reason for hiding this comment

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

Did you forget to remove this line?

System.out.println(line);
}

public boolean chat(String s) throws IncorrectInputException, IOException {

Choose a reason for hiding this comment

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

Maybe extract the components in this method into different methods? There is a lot of information in just 1 method.

public static void main(String[] args) throws IOException, IncorrectInputException {
new Duke("src/main/java/tasklist.txt").run();
}
// public static void main(String[] args) {

Choose a reason for hiding this comment

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

Remember to delete this if you're not using it in your code!

* @throws IOException
* @throws IncorrectInputException
*/
public void parse(String s) throws IOException, IncorrectInputException {

Choose a reason for hiding this comment

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

Maybe extract out some parts of this method to shorten it? This method may become even longer as more functions of the application is added later on.

@@ -0,0 +1,84 @@
import java.io.*;

Choose a reason for hiding this comment

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

Maybe explicitly importing each line rather than using .* would be clearer?

Copy link

Choose a reason for hiding this comment

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

Perhaps you may avoid the usage of * in import statements because it will help you and other developers easily understand the code base when all the classes have been grouped in packages.

@@ -0,0 +1,45 @@
public class Task {

private boolean completed;

Choose a reason for hiding this comment

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

Maybe changed the naming of this variable to isCompleted?

Copy link

@marcustw marcustw 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! Overall code follows good Code Quality examples from the textbook, except for some methods may be too long. Well done.

@@ -0,0 +1,10 @@

enum Chat {
Copy link

Choose a reason for hiding this comment

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

Enums are constants so perhaps it is fine to be fully capitalised.

@@ -0,0 +1,145 @@
import java.io.*;
Copy link

Choose a reason for hiding this comment

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

Perhaps you may avoid the usage of * in import statements because it will help you and other developers easily understand the code base when all the classes have been grouped in packages.

import java.time.temporal.ChronoUnit;

public class Chatbot {
public String line = "____________________________________________________________";
Copy link

Choose a reason for hiding this comment

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

Perhaps the variable line can be final, with capitalised variable name as a constant?

System.out.println(t.printTask());
System.out.println("Now you have " + list.size() + (list.size() > 1 ? " tasks" : " task") + " on the list");
System.out.println(line);
}
Copy link

Choose a reason for hiding this comment

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

Maybe you can leave a line between methods? (between line 30 and line 31)

System.out.println("Nice! I've marked this task as done:");
System.out.println(t.printTask());
System.out.println(line);

Copy link

Choose a reason for hiding this comment

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

Perhaps this empty space at line 80 can be removed?

bot.greeting();


while (sc.hasNext() && bot.chat(sc.nextLine())) {
Copy link

Choose a reason for hiding this comment

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

Perhaps bot.chat(sc.nextLine()) can be added under the while loop instead of the conditional cases to make the while loop clearer?

* @throws IOException
* @throws IncorrectInputException
*/
public void parse(String s) throws IOException, IncorrectInputException {
Copy link

Choose a reason for hiding this comment

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

Perhaps as Jian Ling mentioned, you can extract out the methods and break the long chunk of code down into smaller methods to make this method cleaner?

@@ -0,0 +1,84 @@
import java.io.*;
Copy link

Choose a reason for hiding this comment

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

Perhaps you may avoid the usage of * in import statements because it will help you and other developers easily understand the code base when all the classes have been grouped in packages.

}

/**
* Mark the task as completed
Copy link

Choose a reason for hiding this comment

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

Perhaps the description of javadocs can start with "Marks" instead of "Mark"?

Copy link

@ramenmen ramenmen left a comment

Choose a reason for hiding this comment

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

Overall I don't think I saw many coding standard violations! Nice code!!

@@ -0,0 +1,145 @@
import java.io.*;
Copy link

Choose a reason for hiding this comment

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

Maybe imports can be listed explicitly?

switch (firstWord) {
case "bye":
exit();
return false;
Copy link

Choose a reason for hiding this comment

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

Maybe you could add a line break after each case to increase readability?


/**
* Parse users' commands and respond to them.
* @param s
Copy link

Choose a reason for hiding this comment

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

Should a line be left between the description and @param?

return "[" + this.getStatusIcon() + "] " + this.name;
}

public int completedAsNumber() {
Copy link

Choose a reason for hiding this comment

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

Maybe this method name could be written as a verb, like convertCompletedToNumber or something?

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.

None yet

4 participants