Skip to content

[vimalapugazhan] iP#188

Open
vimalapugazhan wants to merge 48 commits into
nus-cs2113-AY2324S2:masterfrom
vimalapugazhan:master
Open

[vimalapugazhan] iP#188
vimalapugazhan wants to merge 48 commits into
nus-cs2113-AY2324S2:masterfrom
vimalapugazhan:master

Conversation

@vimalapugazhan

Copy link
Copy Markdown

No description provided.

Comment thread src/main/java/Aragorn.java Outdated
index = Integer.parseInt(userInput.substring(5)) - 1;
list[index].markAsDone();
System.out.println(LINE + TAB + "Nice! I've marked this task as done:\n" + TAB +
" [X] " + list[index].getDescription() +"\n" + 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.

Missing a space between + and "\n"

Comment thread src/main/java/Aragorn.java Outdated
return;
}

if (userInput.equals("list")) {

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 refactoring the code to reduce the length in void main

Comment thread src/main/java/Aragorn.java Outdated
continue;
}

else if (userInput.contains("mark")) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

else if should be placed on the same line as on 42

Comment thread src/main/java/Aragorn.java Outdated
@@ -0,0 +1,58 @@
import java.util.Scanner;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Import was explicitly listed

Comment thread src/main/java/Aragorn.java Outdated
String userInput = in.nextLine();

if (userInput.equals("bye")) {
System.out.println(LINE + TAB + EXIT + 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.

consider using multiple print statements to make the code more readable

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

public String getStatusIcon() {
return (isDone ? "X" : " "); // mark done task with X

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ideal to refer to all instance parameters with the this keyword, e.g. this.isDone

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

public String getStatusIcon() {
return (isDone ? "X" : " "); // mark done task with X

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

good use of ternary statements, though brackets are not needed to evaluate them

Comment thread src/main/java/Aragorn.java Outdated
" What can I do for you?\n";
String EXIT = " Bye. Hope to see you again soon!\n";
String TAB = " ";
Task[] list = new Task[100];

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 making the length of the task array a variable

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

well done, i've pointed out a few things that can improve the readability and quality of your code. 👍🏽

Comment thread src/main/java/commands/inputParser.java Outdated

import exceptions.AragornException;

public class inputParser {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Class/enum names must be nouns and written in PascalCase. So in your case, it should be InputParser instead.

Comment thread src/main/java/commands/inputParser.java Outdated
import exceptions.AragornException;

public class inputParser {
private String[] splitInput = new String[3];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

avoid using magic literals in the code, consider storing it in a constant (static final)

https://nus-cs2113-ay2324s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-magic-numbers

Comment thread src/main/java/commands/inputParser.java Outdated
public inputParser(String userInput, String commandType) throws AragornException {
String[] splitDeadline;
String[] splitEvent;
String LINE = " __________________________________________________________\n";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

good, but maybe this can be a constant at the class-level

Comment thread src/main/java/commands/inputParser.java Outdated
public class inputParser {
private String[] splitInput = new String[3];

public inputParser(String userInput, String commandType) throws AragornException {

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 function is quite long (30+ LOC). consider abstracting out the code into multiple functions

https://nus-cs2113-ay2324s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-long-methods

Comment thread src/main/java/commands/inputParser.java Outdated
Comment on lines +13 to +38
case "MARK":
this.splitInput[0] = String.valueOf(Integer.parseInt(userInput.substring(5).trim()) - 1);
this.splitInput[1] = null;
this.splitInput[2] = null;
break;

case "UNMARK":
this.splitInput[0] = String.valueOf(Integer.parseInt(userInput.substring(7).trim()) - 1);
this.splitInput[1] = null;
this.splitInput[2] = null;
break;

case "TODO":
try {
this.splitInput[0] = userInput.substring(4);
if (this.splitInput[0].trim().isEmpty()){
throw new AragornException(LINE + " Task description is empty!\n" + LINE);
}
this.splitInput[1] = null;
this.splitInput[2] = null;
} catch (AragornException e) {
System.out.println(e.getMessage());
}
break;

case "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.

the command types can perhaps be stored as constants or you could probably use enums to avoid the usage of magic literals.

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

import exceptions.AragornException;
import commands.inputParser;
import 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.

avoid using wildcard imports

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

public class Aragorn {

private static final String LINE = " __________________________________________________________\n";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

good use of constants

Comment thread src/main/java/main/Aragorn.java Outdated
Comment on lines +43 to +52
while(true) {
String userInput = in.nextLine();
String commandType = inputParser.commandIdentifier(userInput);
try {
inputParser input = new inputParser(userInput.trim(), commandType);


switch (commandType) {
case "LIST":
if (listLength == 0) {

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 is 4 levels of nesting, it makes the code hard to read and debug if any problems arise. consider refactoring this to multiple methods.

"\n" +
" \"bye\": Closes the program.\n";

public static void main(String[] args) throws AragornException {

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 function is also very long (>30 LOC)

Comment thread src/main/java/tasks/Deadline.java Outdated
public class Deadline extends Task {

public String deadline;
//public static String taskType = "D";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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