-
Notifications
You must be signed in to change notification settings - Fork 73
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
[Nigelle Leo] iP #47
base: master
Are you sure you want to change the base?
[Nigelle Leo] iP #47
Conversation
Add toolVersion block in to Gradle code sample to prevent errors.
Change file mode on `gradle` to be executable (nus-cs2113-AY1920S2#9)
Gradle defaults to an empty stdin which results in runtime exceptions when attempting to read from `System.in`. Let's add some sensible defaults for students who may still need to work with the standard input stream.
Add configuration for console applications
The OpenJFX plugin expects applications to be modular and bundled with jlink, resulting in fat jars that are not cross-platform. Let's manually include the required dependencies so that shadow can package them properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! Code is easy to understand and to follow overall.
src/main/java/Duke.java
Outdated
printWelcomeMessage(); | ||
|
||
Task[] tasks = new Task[CAPACITY]; | ||
int counter = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better if this variable was more specific? (ie taskCounter instead of counter)
src/main/java/Duke.java
Outdated
|
||
while(true) { | ||
String line; | ||
Scanner input = new Scanner(System.in); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could move the Scanner object out of the while loop as it is being reinitialised on every iteration of the loop
src/main/java/Duke.java
Outdated
System.out.println("____________________________________________________________"); | ||
System.out.println("Here are the tasks in your list:"); | ||
for (int i = 0; i < counter; i++){ | ||
System.out.println(i+1 + ". " + tasks[i].toString()); | ||
} | ||
System.out.println("____________________________________________________________"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can try abstracting this out as a separate method to improve readability. Also, would it be better if you used a constant/class level variable to encapsulate the strings (on lines 21 and 26) to prevent magic strings?
src/main/java/Duke.java
Outdated
} | ||
System.out.println("____________________________________________________________"); | ||
} else if (line.startsWith("todo")) { | ||
line = line.replaceAll("todo ", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May cause bugs if there are multiple todo's within the user input (e.g. "todo finish methods for todo deadline event")
src/main/java/Duke.java
Outdated
String description = words[0].substring(9); | ||
String by = words[1].substring(3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could replace the .substring() parameters with constants, as the 3 and 9 can be considered magic numbers.
src/main/java/Duke.java
Outdated
counter++; | ||
} else if (line.startsWith("done")) { | ||
String strNumber = line.substring(5); | ||
int number = Integer.parseInt(strNumber); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to rename this variable to be more specific? (e.g. taskNumber)
src/main/java/Duke.java
Outdated
public static void main(String[] args) { | ||
printWelcomeMessage(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of abstraction to improve code quality
src/main/java/Duke.java
Outdated
line = input.nextLine(); | ||
|
||
if (line.equalsIgnoreCase("bye")) { | ||
printByeMessage(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of abstraction to improve code quality
src/main/java/Duke.java
Outdated
} else if (line.startsWith("event")) { | ||
String[] words = line.split("/"); | ||
String description = words[0].substring(6); | ||
String at = words[1].substring(3); | ||
Task e = new Event(description, at); | ||
tasks[counter] = e; | ||
printAcknowledgement(tasks[counter], counter); | ||
counter++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could consider changing your else-if statements for "task", "todo", "deadline" and "event" to a switch instead and set the parameter as the first word in the input
Add Increment: Level-8
Add Increment: Level-9
Add Increment: A-JavaDoc
No description provided.