-
Notifications
You must be signed in to change notification settings - Fork 187
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
[YoengKokLeong] iP #176
base: master
Are you sure you want to change the base?
[YoengKokLeong] iP #176
Conversation
public static void main(String[] args) { | ||
String logo = " ____ _ \n" | ||
public static void addList(String line) { | ||
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.
I feel like using a constant name variable is better in this case so that the entire presentation feels cleaner without all the dashes. Such as using static String BREAK_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.
Good job on all the naming conventions! :)
src/main/java/Duke.java
Outdated
|
||
introStart(); //Prints starting screen | ||
|
||
boolean ifExit = false; //exits program if true |
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.
I think you can consider renaming the boolean variable since the word "ifExit" might not sound like a boolean variable name.
src/main/java/Duke.java
Outdated
while (!ifExit) { | ||
String line = in.nextLine(); | ||
if (line.equals("bye")) { | ||
//checks for bye interaction and sets ifExit to true |
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 for writing comments for easier understanding of your codes! :)
src/main/java/Duke.java
Outdated
|
||
introStart(); //Prints starting screen | ||
|
||
boolean ifExit = false; //exits program if true |
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.
does not sound like a boolean variable
@@ -1,10 +1,85 @@ | |||
import java.util.Scanner; | |||
import java.util.Arrays; | |||
|
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 good naming. names are self explanatory
} | ||
|
||
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.
the code here is very long. could use some commenting to make code easier to read.
src/main/java/Duke.java
Outdated
|
||
while (!ifExit) { | ||
String line = in.nextLine(); | ||
if (line.equals("bye")) { |
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.
conditional statements are clear. not a lot of nested if-else statements
@@ -1,10 +1,85 @@ | |||
import java.util.Scanner; | |||
import java.util.Arrays; | |||
|
|||
public class Duke { |
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.
indentations LGTM
…ch. Added documentation as well
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.
Looks oki! Just some minor coding standard violation
src/main/java/Duke.java
Outdated
+ "| |_| | |_| | < __/\n" | ||
+ "|____/ \\__,_|_|\\_\\___|\n"; | ||
static boolean ifExit = false; //exits program if true | ||
static Task[] tasks = new Task[100]; //List of tasks |
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.
just a suggestion: the value 100 can be placed as a named constant so that it's easier for you to change this value (e.g. dh to keep scrolling down the code)
src/main/java/Duke.java
Outdated
echoTask(); | ||
break; | ||
case "event": //add a new event task | ||
tasks[listIndex] = new Event(line.substring(0, eventDividerPositionFrom).trim(), line.substring(eventDividerPositionFrom + 5,eventDividerPositionTo).trim(), line.substring(eventDividerPositionTo + 3).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.
Line length should be no longer than 120 chars
src/main/java/Duke.java
Outdated
echoTask(); | ||
break; | ||
case "deadline": //add a new deadline task | ||
tasks[listIndex] = new Deadline(line.substring(0, deadlineDividerPositionBy).trim(), line.substring(deadlineDividerPositionBy + 3).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.
Line length should be no longer than 120 chars
src/main/java/Duke.java
Outdated
{ | ||
int printCounter = 1; | ||
if (listIndex == 0) { | ||
throw new IllegalShapeException(); //Throws exception for empty list | ||
} | ||
System.out.println("--------------------------------------"); | ||
System.out.println("Here are the tasks in your lists:"); | ||
for (Task item : Arrays.copyOf(tasks, listIndex)) { | ||
System.out.print(printCounter + "."); | ||
System.out.println(item); | ||
printCounter++; | ||
} | ||
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.
are these extra brackets? (line 25 and 38)
Added "find" feature
added JavaDoc for classes
Week 4