-
Notifications
You must be signed in to change notification settings - Fork 315
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
[Tan Wei, Adam] Duke Increments #361
base: master
Are you sure you want to change the base?
[Tan Wei, Adam] Duke Increments #361
Conversation
Add toolVersion block in to Gradle code sample to prevent errors.
Change file mode on `gradle` to be executable (nus-cs2103-AY1920S1#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
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.
Hello! I think you can break up Duke.java into several different classes to help make future modifications smoother.
src/main/java/Duke.java
Outdated
break; | ||
case "todo": | ||
case "event": | ||
case "deadline": |
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.
ohhhh, this seems interesting, how does switch case work in this scenario? ^-^
import static org.junit.jupiter.api.Assertions.*; | ||
|
||
class DukeTest { | ||
|
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.
Did you forget to include the body?
public ListItem(String description, String command) { | ||
try { | ||
switch (command) { | ||
case "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.
Clear logic flow, but perhaps you can consider separating the different task types using different classes?
src/main/java/Duke.java
Outdated
} | ||
//try to catch more exceptions | ||
catch (Exception e) { | ||
dukePrint("☹ OOPS!!! The description of a " + userCommand + " cannot be empty."); |
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 might want to create a seperate DukeException class for this later on!
boolean exists = false; | ||
String precursor = ""; | ||
SimpleDateFormat formatter = new SimpleDateFormat("dd/MM/yy HHss"); | ||
java.util.Date data; |
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.
Putting the date import at the start of the file as per convention would be better practice!
String precursor = ""; | ||
SimpleDateFormat formatter = new SimpleDateFormat("dd/MM/yy HHss"); | ||
java.util.Date data; | ||
public Date(String dateString) { |
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.
Could write javadocs for the methods!
public String toString() { | ||
if (exists) { | ||
return "(" + precursor + " " + formatter.format(data) + ")"; | ||
} |
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.
Try to conform to the 2103 coding style!
the else { should be on the same line as '}'
* Date representation of date string that's given | ||
*/ | ||
public class Date { | ||
boolean exists = false; |
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.
Naming of boolean could be more semantic
* Prints what Duke says in correct format. | ||
* @param texts any number of String arguments | ||
* each prints on a new 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.
Unused javadoc could be removed.
/** | ||
* Main driver class for Duke. | ||
* | ||
*/ | ||
public static void main(String[] args) { |
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.
Remove unused main method
ListItem(String description, String command, String date) { | ||
try { | ||
switch (command) { | ||
case "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.
Try to conform to the coding style! ( "case ..." should be on the same indent level as the switch statement )
@@ -0,0 +1,42 @@ | |||
import java.text.SimpleDateFormat; |
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 choice to create a Date class, because it is easier to edit when you change the Date format.
java.util.Date data; | ||
public Date(String dateString) { | ||
precursor = dateString.split(" ", 2)[0]; | ||
if (!dateString.equals(" ")) { |
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.
Instead of this, use SLAP to abstract out the checking method.
private Button sendButton; | ||
private Scene scene; | ||
|
||
private Image user = new Image(this.getClass().getResourceAsStream("/images/User.jpg")); |
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 consider storing the image path as a private static string to make it neater.
private static void echo(String echoedString) { | ||
dukePrint(echoedString); | ||
} | ||
|
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.
This should be in UI.
while(reader.hasNext()) { | ||
String commandIn = reader.nextLine(); | ||
String[] commandArray = commandIn.split("@"); | ||
tempArray.add(new ListItem(commandArray[2], commandArray[1], commandArray[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.
After you split the command, you can consider declaring each array item as a String for better understanding later, rather than commandArray[x]
*/ | ||
void save(ArrayList<ListItem> toSave) { | ||
try { | ||
BufferedWriter writer = new BufferedWriter(new FileWriter(filepath)); |
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.
Rmb to close BufferedWriter!
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.
Overall very creative code! We had a pleasant time reading your code! cheeers yeeeeeeeeeeeeeeeeet
else { | ||
date = " "; | ||
} | ||
switch(userCommand) { |
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.
Your switch case formatting does not comply with the java coding standard. Try to change it like this (the case is not indented):
switch(something) {
case "x":
return x;
case "y":
return y;
}
No description provided.