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

[ThienDuc3112] iP #100

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

Conversation

ThienDuc3112
Copy link

No description provided.

@@ -0,0 +1,111 @@
import java.util.*;

Choose a reason for hiding this comment

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

Avoid using wildcard imports
Import only the functions that you need

isOn = true;
datas = new ArrayList<>();

String greeting = "____________________________________________________________\n" +

Choose a reason for hiding this comment

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

use "final" and SCREAMING_SNAKE_CASE for the greeting variable as it is a constant

return isDone;
}

public void mark(){

Choose a reason for hiding this comment

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

add a space before the "{" to keep layout consistent

String line = in.nextLine().strip();
System.out.println("____________________________________________________________");

if (line.equals("bye")) {

Choose a reason for hiding this comment

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

Followed if else layout convention. Good job

Copy link

@seanngja seanngja left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -0,0 +1,111 @@
import java.util.*;
Copy link

Choose a reason for hiding this comment

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

Perhaps you could import just the libraries you need?

Comment on lines 26 to 40
} else if (line.equals("list")) {
list();
} else if (line.startsWith("mark ")) {
mark(line);
} else if (line.startsWith("unmark ")) {
unmark(line);
} else if (line.startsWith("todo ")) {
todo(line);
} else if (line.startsWith("event ")) {
event(line);
} else if (line.startsWith("deadline ")) {
deadline(line);
} else {
add(line);
}
Copy link

Choose a reason for hiding this comment

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

Perhaps you could refactor this into another function? This chunk is long and hard to read.


private static void todo(String line) {
String todo = line.substring(line.indexOf(" ") + 1).trim();
Todo task =new Todo(todo);
Copy link

Choose a reason for hiding this comment

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

Fix the formatting after the = sign.


public static void main(String[] args) {
isOn = true;
datas = new ArrayList<>();
Copy link

Choose a reason for hiding this comment

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

Perhaps you could use a more descriptive name for your array list, as it is unclear what is being stored here.

Comment on lines +30 to +33
return "[" +
(isDone ? "X" : " ") +
"] " +
task;
Copy link

Choose a reason for hiding this comment

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

Perhaps you could write this all in 1 line of code to make your code more readable.

Copy link

@Mahesh1772 Mahesh1772 left a comment

Choose a reason for hiding this comment

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

Solid effort

Code is following most of the code quality standards. The naming would need to be improved for better scalability so that fellow developers can understand the use of the method and variable easily. Consider adding JavaDocs in future commits

private static List<Task> taskList;

public static void main(String[] args) {
isOn = true;

Choose a reason for hiding this comment

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

Consider using more descriptive names, that is easily identified to serve a definite purpose

Comment on lines 27 to 42
isOn = false;
} else if (line.equals("list")) {
list();
} else if (line.startsWith("mark ")) {
mark(line);
} else if (line.startsWith("unmark ")) {
unmark(line);
} else if (line.startsWith("todo ")) {
todo(line);
} else if (line.startsWith("event ")) {
event(line);
} else if (line.startsWith("deadline ")) {
deadline(line);
} else {
System.out.println("Unknown command: " + line);
}

Choose a reason for hiding this comment

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

Consider using switch case for this case, as the different cases of input is pre-determined

}
}

private static void list() {

Choose a reason for hiding this comment

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

Consider naming it as listTasks to avoid confusion with the inbuilt type

Comment on lines 57 to 74
int option = Integer.parseInt(line.substring(line.indexOf(" ") + 1));
System.out.println("Nice! I've marked this task as done:");
taskList.get(option - 1).mark();
System.out.println(taskList.get(option - 1).toString());
} catch (Exception e) {
System.out.println("Invalid input, input must be a positive integer and must exist");
}
}

private static void unmark(String line) {
try {
int option = Integer.parseInt(line.substring(line.indexOf(" ") + 1));
System.out.println("OK, I've marked this task as not done yet:");
taskList.get(option - 1).unmark();
System.out.println(taskList.get(option - 1).toString());
} catch (Exception e) {
System.out.println("Invalid input, input must be a positive integer and must exist");
}

Choose a reason for hiding this comment

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

Consider using the same method or a central part for both functions as to reduce the number of duplicate lines of code

System.out.println("Now you have " + taskList.size() + " tasks in the list.");
}

private static void event(String line) {

Choose a reason for hiding this comment

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

Consider naming the methods better for easy maintainability and scalability

Comment on lines +2 to +3
String from;
String to;

Choose a reason for hiding this comment

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

Consider something like fromDate , toDate

Comment on lines +31 to +34
return "[" +
(isDone ? "X" : " ") +
"] " +
task;

Choose a reason for hiding this comment

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

Consider same line implementation to increase code readability


public class Utilities {
public static Hashtable<String, String> getCommandArgument(String line) {
Hashtable<String, String> args = new Hashtable<>();

Choose a reason for hiding this comment

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

Try to not use vague names for variables

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