-
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
[Mohamed Nizar Bin Mohamed Hussain] iP #69
base: master
Are you sure you want to change the base?
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.
All the best for your IP ;-)
Overall, we found your code easy to read.
[Also took part in this review: @NyanWunPaing]
src/main/java/Duke.java
Outdated
for (i = 0; i < l1.size(); i++) { | ||
int count = i + 1; | ||
Task temp = l1.get(i); | ||
System.out.println("\t " + count +"."+ temp.getTaskDescription().trim()); |
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.
Shouldn't the spaces between the string and operator be more consistent ? 🤔
System.out.println("\t " + count + "." + temp.getTaskDescription().trim());
src/main/java/Duke.java
Outdated
System.out.println("\t Nice! I've marked this task as done:"); | ||
System.out.println("\t "+tempTask.getTaskDescription()); | ||
System.out.println(LINE); | ||
}else{ |
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.
shouldnt this be
} else {
🤔
src/main/java/Duke.java
Outdated
String[] temp=getCommand(userIn); | ||
String action=""; | ||
boolean flip=false; | ||
for(int i=1; i<(temp.length); i++) { | ||
if (temp[i].charAt(0) == '/') { | ||
flip=true; | ||
} | ||
if(flip){ | ||
timing += temp[i] + " "; | ||
}else{ | ||
action += temp[i] + " "; |
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.
Shouldn't there be spaces before and after the equal sign? It is good to be consistent..
src/main/java/Duke.java
Outdated
String banner = "\t~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n" | ||
+ "\t~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n" | ||
+ "\t~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n" | ||
+ "\t~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n" | ||
+ "\t~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\n"; |
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.
Make this a constant like the constant LINE?
src/main/java/Duke.java
Outdated
default: | ||
status = -1; |
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 include a break or //Fallthrough in the default case? 💭
This is recorded in the coding standard:
"The explicit //Fallthrough comment should be included whenever there is a case statement without a break statement."
src/main/java/Duke.java
Outdated
public class Duke { | ||
public static final String LINE = "\t__________________________________________________________"; | ||
public static final String[] COMMAND= {"todo", "deadline", "event", "done", "bye", "help"}; |
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 define an enumeration type call Command that has six values TODO, DEADLINE, EVENT, DONE, BYE, HELP?
User Guide for Nini V2.0
No description provided.