-
Notifications
You must be signed in to change notification settings - Fork 77
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
Syed Omar Zoran iP #67
base: master
Are you sure you want to change the base?
Conversation
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 ok overall, just small coding standard problems
src/main/java/Duke.java
Outdated
@@ -1,28 +1,48 @@ | |||
import java.util.Scanner; | |||
public class Duke { | |||
Tasks taskList[] = new Tasks[100]; |
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 avoid using magic numbers?
src/main/java/Duke.java
Outdated
void printWelcome() { | ||
System.out.println("Hello I'm Duke\nWhat can I do for you?"); | ||
} | ||
void acceptCommands() { | ||
Scanner input = new Scanner(System.in); | ||
String command; | ||
String commandList[] = new String[100]; | ||
int noOfCommands = 0; | ||
do { | ||
command = input.nextLine(); | ||
if(command.equals("list")) { |
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.
Perhaps you can add a space between "if" and "("
src/main/java/Duke.java
Outdated
unmarkTasks(Integer.parseInt(command.substring(command.lastIndexOf(' ') + 1))); | ||
continue; | ||
} | ||
|
||
System.out.println("added: "+command); |
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 add spaces between "added: ", "+" and "command"
src/main/java/Tasks.java
Outdated
public class Tasks { | ||
String taskName; | ||
boolean done; | ||
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.
Maybe you could add a "public " before the 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.
Overall great effort. Try to put white space in between expressions and make your function name more meaningful to the task it perform.
src/main/java/Duke.java
Outdated
} | ||
void markTasks(int taskNumber) { | ||
System.out.println("Nice! I've marked this task as done:"); | ||
taskList[taskNumber-1].setDone(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.
Try to put a space between previous and next text.
src/main/java/Duke.java
Outdated
continue; | ||
} | ||
if(command.indexOf(' ') > 0 && command.substring(0,command.indexOf(' ')).equals("mark")) { | ||
markTasks(Integer.parseInt(command.substring(command.lastIndexOf(' ') + 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.
Since you're only marking one task, don't have to put plural. markTask is suffice.
src/main/java/Duke.java
Outdated
displayList(noOfCommands); | ||
continue; | ||
} | ||
if(command.indexOf(' ') > 0 && command.substring(0,command.indexOf(' ')).equals("mark")) { |
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 add a variable to it. Reading the statement can be difficult especially when the condition is long.
src/main/java/Duke.java
Outdated
taskList[noOfCommands] = new Tasks(); | ||
taskList[noOfCommands++].setTaskName(command); | ||
} | ||
while(!command.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.
Try to have a variable for this boolean expression to be more intuitive.
src/main/java/Duke.java
Outdated
void printWelcome() { | ||
System.out.println("Hello I'm Duke\nWhat can I do for you?"); | ||
} | ||
void acceptCommands() { |
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.
Since you're only accepting 1 command, don't have to put plural. acceptCommand is sufficient.
* commit 'a0d42feff8f25a421e59ee0399f305e68f2673b4': Change exception message
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 work on the implementation! There are some repeated coding standard violations so do be mindful of those. Consider refactoring where needed to improve readability.
src/main/java/Duke/Deadline.java
Outdated
void initializeMessage() { | ||
System.out.println("Got it! I've added this task:"); | ||
displayMessage(); | ||
System.out.println("Now you have "+ TaskManager.noOfTasks +" tasks in the list."); |
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.
As per the Java coding standard it is recommended to have white spaces before and after your "+" operator.
src/main/java/Duke/Tasks.java
Outdated
return taskName; | ||
} | ||
|
||
public String taskDoneStatus() { |
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.
Names representing methods should be verbs, as per the Java coding standard. getTaskDoneStatus() would be better.
src/main/java/Duke/Tasks.java
Outdated
|
||
public class Tasks { | ||
String taskName; | ||
boolean done; |
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.
Booleans should be made to sound like booleans, as per the Java coding standard.
src/main/java/Duke/Duke.java
Outdated
} | ||
|
||
void acceptInput() { | ||
Scanner SC = 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.
Variable names must be in camelCase, fully uppercase names are reserved for constants.
src/main/java/Duke/Duke.java
Outdated
|
||
void acceptInput() { | ||
Scanner SC = new Scanner(System.in); | ||
TaskManager TM = new TaskManager(); |
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.
As mentioned above, variable names should be in camelCase.
src/main/java/Duke/Event.java
Outdated
void initializeMessage() { | ||
System.out.println("Got it! I've added this task:"); | ||
displayMessage(); | ||
System.out.println("Now you have "+ TaskManager.noOfTasks +" tasks in the list."); |
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.
Operators should be surrounded by a space character, as per the Java coding standard.
void notRecognizedException() { | ||
System.out.println("OOPS!!! I'm sorry, but I don't know what that means :-("); | ||
} | ||
void taskOutOfBounds() { |
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.
Can be renamed to taskOutofBoundsException to be consistent with the other methods here.
Code Quality Guideline: Name Well
package Duke; | ||
|
||
public class DukeException { | ||
void todoException() { |
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.
Interesting use of methods to display exception messages. Since these are methods, their naming should be represented using verbs. throwTodoException() and so on would be better.
Add java-doc documentation
No description provided.