Skip to content

[Liam Van] IP#199

Open
SpeciLiam wants to merge 27 commits into
nus-cs2113-AY2223S2:masterfrom
SpeciLiam:master
Open

[Liam Van] IP#199
SpeciLiam wants to merge 27 commits into
nus-cs2113-AY2223S2:masterfrom
SpeciLiam:master

Conversation

@SpeciLiam

Copy link
Copy Markdown

No description provided.

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

Nice, just nitpicking

Comment thread src/main/java/Duke.java Outdated
public static void main(String[] args) {

Duke duke = new Duke();
ArrayList<String> userInputs = new ArrayList<>();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Like your naming for the ArrayList!

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

}
exit();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just nitpicking, but would be nice if there was a space after this :^)

Comment thread src/main/java/Duke.java Outdated
}
public static void markDone(int index, ArrayList<String> userInputs, boolean isMark) {
if (isMark) {
String org = userInputs.get(index - 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.

I think it would be better to use a more descriptive name -- not sure what org means here

Comment thread src/main/java/duke/Duke.java Outdated
Main function that takes user input and interpets how to store and what to do with it
*/
public static void main(String[] args) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do avoid leaving spaces like this as it goes against the Java Coding Standards

Comment thread src/main/java/duke/Duke.java Outdated
Comment on lines +16 to +18
public Duke() {

}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do consider removing empty methods, if not you could refactor some of your code into this. :)

Comment thread src/main/java/duke/Duke.java Outdated
+ "| | | | | | | |/ / _ \\\n"
+ "| |_| | |_| | < __/\n"
+ "|____/ \\__,_|_|\\_\\___|\n";
System.out.println("Hello from\n" + logo);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I notice that you have a tendency to use magic strings and numbers. Do try to avoid that where possible, and instead use constants to set your strings/numbers.

Comment thread src/main/java/duke/Duke.java Outdated
System.out.println("\t____________________________________________________________");
}
/*
This Returns the input as a Deadline object

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please remove such comments as they go against the java coding standards.

import dukeException.DukeException;

public class Todo extends Task {
public Todo(String description, boolean isMark) throws DukeException {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please do remove the extra space between ) and throws!

@@ -0,0 +1,5 @@
package dukeException;

public class DukeException extends Throwable {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Try not to leave your exceptions empty. Perhaps you could have the corresponding error message and a method to print/return the error message?

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.

3 participants