Skip to content

[Thomas Cherian] iP#274

Open
tomascherian wants to merge 28 commits into
nus-cs2103-AY2122S2:masterfrom
tomascherian:master
Open

[Thomas Cherian] iP#274
tomascherian wants to merge 28 commits into
nus-cs2103-AY2122S2:masterfrom
tomascherian:master

Conversation

@tomascherian
Copy link
Copy Markdown

No description provided.

Comment thread src/main/java/Deadline.java Outdated
@@ -0,0 +1,17 @@
public class Deadline extends Task{
private final String dateTime;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Var declares as final may need to be in all capital letters, eg: DATE_TIME

Comment thread src/main/java/Event.java Outdated
@@ -0,0 +1,17 @@
public class Event extends Task {
private final String dateTime;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

final var too 😄

Comment thread src/main/java/Pac.java Outdated
String input = sc.nextLine();
try {
String[] inputArray = input.split(" ", 2);
String firstword = inputArray[0].toLowerCase();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

name as firstWord maybe better

Comment thread src/main/java/Pac.java Outdated
default:
keyword = Keyword.ERROR;
break;
}
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 using a hashmap to lookup the right Keyword given a string? same functionality but may save some lines, may look slightly more readable as well, but it's up to you

Comment thread src/main/java/Pac.java Outdated
break;
case ERROR:
throw new PacException("I don't know what this means");
}
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 rewriting these lines of code into separate functions to avoid deep nesting

Copy link
Copy Markdown

@manu2002g manu2002g left a comment

Choose a reason for hiding this comment

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

I noticed that there aren't any JavaDoc comments and maybe you should consider adding them to make your code easier to understand.
But overall great code quality that is quite readable! :)

Comment thread src/main/java/Pac.java Outdated
Comment on lines +5 to +10
static String logo = " ____ ___ _____\n"
+ "| _ \\ / _ \\ | ___|\n"
+ "| |_| | | |_| | | |\n"
+ "| __/ | | | | | |___\n"
+ "|_| |_| |_| |_____|\n";
static String newline = "----------------------------------------------------";
Copy link
Copy Markdown

@manu2002g manu2002g Feb 5, 2022

Choose a reason for hiding this comment

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

Should the logo and newline variables be made final? And their names changed to fit the coding standard for naming constants eg: LOGO

Comment thread src/main/java/Pac.java Outdated
String firstword = inputArray[0].toLowerCase();
Keyword keyword;

switch (firstword) {
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 coding standard says that we shouldn't indent the case statements under switch I think it's possible to configure IntelliJ to follow this by default.

Comment thread src/main/java/Pac.java Outdated
break;
}

switch (keyword) {
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 case statements here should also not be intended

Comment thread src/main/java/Pac.java Outdated
Comment on lines +160 to +171
String[] inputArray = input.split("/");
inputArray[1] = inputArray[1].replaceFirst("BY ", "by ");
inputArray[1] = inputArray[1].replaceFirst("By ", "by ");
inputArray[1] = inputArray[1].replaceFirst("bY ", "by ");
inputArray[1] = inputArray[1].split("by ", 2)[1];
if (inputArray[1].isBlank()) {
throw new PacException("Please enter a valid date or time.");
}
Task task = new Deadline(inputArray[0], inputArray[1]);
tasks.add(task);
System.out.println(newline + "\nadded: " + task.toString());
System.out.println("You have " + tasks.size() + " tasks in your list" + "\n" + newline + "\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.

Maybe you can add a few blank lines in this code block to make it more readable? The coding standard suggest that "Logical units within a block should be separated by one blank line." Perhaps you could add blank lines to separate the code into sections dealing with formatting, processing and output.

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