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

[Yap Joon Siong] iP #186

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

Conversation

yapjoonsiong
Copy link

No description provided.

Copy link

@teoziyiivy teoziyiivy left a comment

Choose a reason for hiding this comment

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

Good code overall! It's cool that your program has random responses, very interesting :)

public class Tasks {
Task list[] = new Task[100];
int listLength = 0;
int tasksIncomplete = 0;

Choose a reason for hiding this comment

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

incompleteTasks might be a better variable name here.

Comment on lines 37 to 58
System.out.println("\tNow you have " + user.listLength + " tasks in the list, " + user.tasksIncomplete + " incomplete tasks");
}
public static void addDeadline(String input, Tasks user) {
user.list[user.listLength] = new Deadline(input.substring(9, input.indexOf("/") - 1), input.substring(input.indexOf("/") + 1));
System.out.println("\t" + addTaskSalutation(input.substring(9, input.indexOf("/") - 1)));
user.listLength ++;
user.tasksIncomplete ++;
System.out.println("\tNow you have " + user.listLength + " tasks in the list, " + user.tasksIncomplete + " incomplete tasks");
}
public static void addEvent(String input, Tasks user) {
user.list[user.listLength] = new Event(input.substring(6, input.indexOf("/") - 1), input.substring(input.indexOf("/") + 1));
System.out.println("\t" + addTaskSalutation(input.substring(6, input.indexOf("/") - 1)));
user.listLength ++;
user.tasksIncomplete ++;
System.out.println("\tNow you have " + user.listLength + " tasks in the list, " + user.tasksIncomplete + " incomplete tasks");
}
public static void markDone(String input, Tasks user) {
int index = Integer.parseInt(input.substring(5)) - 1;
if (index < user.listLength) {
user.list[index].markComplete();
user.tasksIncomplete --;
System.out.println("\tAs you wish sir. I have marked this task as done:\n\t[X] " + user.list[index].item + "\n\tNow you have " + user.tasksIncomplete + " incomplete tasks");

Choose a reason for hiding this comment

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

Line length should be no longer than 120 chars.

" @@. %@, @@ @@ #@( @@&@. #@* @&\n" +
" */**. */. /* ** ./* */ ,*. ,****** ";
greet(logo);
Tasks user = new Tasks();

Choose a reason for hiding this comment

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

A different variable name can be used to show tasks is a plural collection of the user's tasks.

public class Duke {
public static String addTaskSalutation(String item) {

Choose a reason for hiding this comment

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

printTaskSalutation might be a better method name.

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 comments for your method to clarify what it is doing.

System.out.println("\t____________________________________________________________\n");
}

public static void showList(Tasks user) {

Choose a reason for hiding this comment

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

printList might be a better method name.

Copy link

@kahhe kahhe left a comment

Choose a reason for hiding this comment

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

Code quality is quite well-adhered to. No deep nesting or methods with more than 30 LOC!

Comment on lines 23 to 25
public static void line() {
System.out.println("\t____________________________________________________________\n");
}
Copy link

Choose a reason for hiding this comment

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

Consider naming as printLine() rather than line(). Methods' names should be verbs for easy understanding of their purposes.

@@ -0,0 +1,18 @@
public class Task {
protected String item;
protected boolean complete;
Copy link

Choose a reason for hiding this comment

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

Boolean variables should sound like a claim that can be true or false. isComplete may be more appropriate here.

…changes. Load the data from the hard disk when Duke starts up.
* branch-Level-6:
  Add support for deleting tasks from the list.
* branch-Level-7:
  Save the tasks in the hard disk automatically whenever the task list changes. Load the data from the hard disk when Duke starts up.

# Conflicts:
#	src/main/java/Duke.java
public class Deadline extends Task{
protected String by;
protected String originalInput;
public Deadline(String description, String by, String originalInput) {

Choose a reason for hiding this comment

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

To improve code readability, you may wish to leave a line spacing the code block of class attributes and the constructor (Deadline()) here.

Random rand = new Random();
String out = "";
switch(rand.nextInt(3)) {
case 0:

Choose a reason for hiding this comment

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

You are using integers for the switch cases. How about creating more meaningful names for each case?

public class Duke {
public static String addTaskSalutation(String item) {

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 comments for your method to clarify what it is doing.

}
public static void addTodo(String input, Tasks user) {
user.list[user.listLength] = new Todo(input.substring(5));
user.listLength ++;

Choose a reason for hiding this comment

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

The increment operator ++ is conventionally used without character spacing e.g.listLength++. You may wish to edit similar lines of code like this.

System.out.println("\t" + (i + 1) + ". " + user.list[i] + "\n");
}
}
public static void addTodo(String input, Tasks user) {

Choose a reason for hiding this comment

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

You may wish to consider renaming the user parameter. It is strange that the parameter is user of type Tasks, when a task usually refers to object/item rather than a person.

Comment on lines 46 to 48
user.list[user.listLength] = new Deadline(input.substring(9, input.indexOf("/") - 1), input.substring(input.indexOf("/") + 1), input.substring(9));
user.listLength ++;
user.tasksIncomplete ++;

Choose a reason for hiding this comment

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

You have repeated uses of user.list[x] and other variables that keep track of the array position to fill. This makes the code slightly more complicated than necessary.

You could simplify the code further by doing without the array index (in this case listLength).
Try ArrayList so that you could add items to a growing list.

}
}

public static void response(String input, Tasks user) throws IllegalCommandException, IllegalTaskException, DueDateFormatException, MultiMarkDoneException, IOException, DeleteTaskFormatException {

Choose a reason for hiding this comment

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

This method is rather long >30 LoC. How about abstracting out the procedures of each case into methods? Also see the SLAP principle.

public static void response(String input, Tasks user) throws IllegalCommandException, IllegalTaskException, DueDateFormatException, MultiMarkDoneException, IOException, DeleteTaskFormatException {
String[] inputArr = input.split(" ");
switch (inputArr[0]) {
case ("list"):

Choose a reason for hiding this comment

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

The brackets () for each case are unnecessary - see switch statement style.

private static void writeToFile(Tasks user) throws IOException {
FileWriter fw = new FileWriter(save);
for (int i = 0; i < user.listLength; i++) {
fw.write(user.list[i].getType() + " | " + user.list[i].getStatus() + " | " + user.list[i].getOriginalInput() + System.lineSeparator());

Choose a reason for hiding this comment

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

This line is rather long, and could trip up the reader. The guideline is to have <120 chars per line.

Refer to the java string format() method example for another way to format long strings.

import java.io.FileNotFoundException;
import java.nio.file.Files;
import java.nio.file.Paths;

public class Duke {

Choose a reason for hiding this comment

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

Generally, you could consider adding more comments to describe what your methods or classes are doing.

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