-
Notifications
You must be signed in to change notification settings - Fork 205
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
[Nguyen Dang Phuc Nhat] Duke A-Assertions A-CodeQuality C-BetterSearch #167
Conversation
Add toolVersion block in to Gradle code sample to prevent errors.
Change file mode on `gradle` to be executable (nus-cs2103-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.
Code looks like a shamble, but meh
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.
Please go through the detailed comments. Avoid adding trivial comments and commented code segemnts in future work.
@@ -0,0 +1,17 @@ | |||
todo borrow book |
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 can .class files to the gitignore files so that complied files will be ignored
* @param msg | ||
*/ | ||
public InvalidCommandException(String msg) { | ||
super(String.format("'%s' is an invalid command", msg)); |
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 can define these messages as constant variables
/** | ||
* Indicates that an index is out of bound | ||
*/ | ||
public class OutOfBoundMarkingRequestException extends Exception { |
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 that you have very specific Exceptions defined
src/main/java/duke/main/Duke.java
Outdated
public static final String LIST_COMMAND = "list"; | ||
public static final String BYE_COMMAND = "bye"; | ||
|
||
// private static String PADDING = " "; |
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.
Please remove commented code
src/main/java/duke/main/Duke.java
Outdated
private Parser parser; | ||
|
||
/** | ||
* Constructor |
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 can avoid trivial information in the comment.
} | ||
|
||
/** | ||
* Stringify the object |
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.
Same here. It should be Strigifies xxx
src/main/java/duke/task/Task.java
Outdated
protected boolean isDone; | ||
|
||
/** | ||
* Constructor with a name |
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.
Please avoid trivial information in the comments
src/main/java/duke/util/Storage.java
Outdated
} | ||
|
||
/** | ||
* Read all data in filePath and turn them into tasks in storedItems |
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.
Same here. It should be Reads xxx
* @param storedItems | ||
* @throws OutOfBoundMarkingRequestException | ||
*/ | ||
public static String markItemAsDone(int pos, ArrayList<Task> storedItems) throws OutOfBoundMarkingRequestException { |
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 the variable names more descriptive e.g. position
src/main/java/duke/util/ui/Ui.java
Outdated
// System.out.println(String.format("%s%s", PADDING, USELESS_LINE)); | ||
// System.out.println(String.format("%sGreetings! This is %s, and I am your friend!", PADDING, BOT_NAME)); | ||
// System.out.println(String.format("%sYou don't have to be formal. Relax and tell me how I can help you", PADDING)); | ||
// System.out.println(String.format("%s%s", PADDING, USELESS_LINE)); |
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.
Please remove the commented code
Should be satisfactory for now...
No description provided.