Skip to content

[Shyun Yin] iP#66

Open
Shyunyin wants to merge 50 commits into
nus-cs2113-AY2122S2:masterfrom
Shyunyin:master
Open

[Shyun Yin] iP#66
Shyunyin wants to merge 50 commits into
nus-cs2113-AY2122S2:masterfrom
Shyunyin:master

Conversation

@Shyunyin

@Shyunyin Shyunyin commented Feb 2, 2022

Copy link
Copy Markdown

No description provided.

@GlendonNotGlen GlendonNotGlen 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 a great job with the tidy code, could use some renaming for some of the methods to make it clearer. Do consider shifting some of your methods in Eliz.java to the classes instead!

Comment thread src/main/java/Eliz.java Outdated
public class Eliz {
public static void printTasks(Task[] tasks) {
for (int i = 0; i < tasks.length; i++) {
int numToPrint = i + 1;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can consider changing numToPrint to something else, like taskIndex

Comment thread src/main/java/Eliz.java Outdated
}
}

public static void markATask(String line, Task[] tasks) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider using markOneTask instead?

Comment thread src/main/java/Eliz.java Outdated
+ tasks[taskNumInt - 1].description);
}

public static Task createTask(String line) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can change this to a method in your Task.java class instead? so that your main.java will not be as convoluted

Comment thread src/main/java/Eliz.java Outdated
System.out.println("What can I do for you?");
}

public static void markOrUnmark(String line, Task[] tasks, int taskCounter) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider using toggleMarkStatus or toggle as ur method name instead?

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

Great work in overall. May need to make some modifications to achieve Single Level of Abstraction so that your code is more readable.

@@ -0,0 +1,18 @@
public class Deadline extends Task{
private String taskType;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can make it as a constant (static variable).

Comment thread src/main/java/Eliz.java Outdated

public static Task createEvent(String taskType, String line) {
String newDescription = line;
if (line.contains("/")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can consider abstracting these lines out as a new method.

Comment thread src/main/java/Eliz.java Outdated

public static Task createDeadline(String taskType, String line) {
String newDescription = line;
if (line.contains("/")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can consider abstracting these lines out as a new method.

Comment thread src/main/java/Eliz.java Outdated
unmarkATask(line, Arrays.copyOf(tasks, taskCounter));
} else {
System.out.println("Nice! I've marked this task as done:");
markATask(line, Arrays.copyOf(tasks, taskCounter));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can consider creating a TaskList class to store the tasks and the relevant methods so that you don't have to keep passing the tasks around.

Comment thread src/main/java/Eliz.java Outdated
/** add line to todo, deadline, or event by creating the respective object */
Task t = createTask(line);
tasks[taskCounter] = t;
taskCounter++;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can abstract these lines out as addTask method.

Comment thread src/main/java/Eliz.java Outdated
t = createEvent(taskType, splitTwoSections[1]);
break;
default:
return null;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

May cause errors when you have other functions trying to access it.

Debug logic in getInput() method
Add a help function that displays the commands for users to input
# Conflicts:
#	README.md
#	src/main/java/Eliz/Eliz.java
Comment thread src/main/java/Eliz/Eliz.java Outdated
if ((breakTaskNames.length < 2)){
String firstWord = breakTaskNames[0];
switch (firstWord) {
case "list" : printTasks(tasks);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You could begin statements for each case in a new line. See coding standard switch case layout.

Comment thread src/main/java/Eliz/Eliz.java Outdated
Comment on lines +147 to +152
// if (line.substring(line.length() - 1) == " ") { //in the case that the last character is a space
// breakTaskNames[0] = line;
// breakTaskNames[1] = null;
// } else {
// breakTaskNames = line.split(" ", 2);
// }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You could delete unused code blocks and lines across your files to increase overall code quality.

Comment thread src/main/java/Eliz/Eliz.java Outdated
Comment on lines +196 to +208
Task t = createTask(line);
tasks.add(t);
System.out.println("Got it. I've added this task ");
System.out.println(t);
System.out.println("Now you have " + tasks.size() + " tasks in the list.");
/*every time an object is added or edited, append to file*/
try {
writeToFile(filePath, tasks.get(tasks.size()-1).getTaskType() + "|"
+ tasks.get(tasks.size()-1).getStatusIcon() + "|"
+ tasks.get(tasks.size()-1).getDescription() + System.lineSeparator());
} catch (IOException e) {
System.out.println("Something went wrong: " + e.getMessage());
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These lines could be refactored further to ensure SLAP.

Comment thread src/main/java/Eliz/Eliz.java Outdated
System.out.println("Now you have " + tasks.size() + " tasks in the list.");
/*every time an object is added or edited, append to file*/
try {
writeToFile(filePath, tasks.get(tasks.size()-1).getTaskType() + "|"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This line of code has too many parameters that decrease the code readability. How about passing in a Task object as a parameter and letting the method unpack its values?

Comment thread src/main/java/Eliz/Eliz.java Outdated
}
}
}
// if (breakTaskNames[0].equalsIgnoreCase("todo") || breakTaskNames[0].equalsIgnoreCase("deadline")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Omit unused code blocks to increase code quality.

Comment thread src/main/java/Eliz/Eliz.java Outdated
Comment on lines +253 to +254
File f = new File(filePath); //helps to create a file for the give file path
Scanner s = new Scanner(f); //create scanner using file as a source

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You could inserted comments on separate line with same indentation relative to the code. This will not break the logical structure of the code. See commenting conventions.

Comment thread src/main/java/Eliz/Eliz.java Outdated
Scanner in = new Scanner(System.in);
botIntroduction(); //calls the introduction of the bot
line = in.nextLine();
String filePath = "./task_list.txt";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You could define this file path as a constant if it is not being further modified anywhere.

Comment thread src/main/java/Eliz/Eliz.java Outdated
System.out.println("unmark [TASK_LIST_NUMBER]");
}

public static void delete(String line, ArrayList<Task> tasks) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It would help to add javadoc comments here to explain what is deleted.

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