-
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
samuelory iP #173
base: master
Are you sure you want to change the base?
samuelory iP #173
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.
Overall good job on following coding standards
src/main/java/OGF.java
Outdated
int taskNo; | ||
while (true) { | ||
String input = scanner.nextLine(); | ||
switch (input.split(" ")[0]) { |
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.
Consider storing the first word of user input in a variable
switch (input.split(" ")[0]) { | |
String command = input.split(" ")[0] | |
switch (command) { |
src/main/java/OGF.java
Outdated
|
||
|
||
Task[] tasks = new Task[100]; | ||
int numItem = 0; |
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.
numberOfTasks might have been a better variable name
src/main/java/Task.java
Outdated
@@ -0,0 +1,38 @@ | |||
public class Task { |
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 like that the boolean names sound booleany
src/main/java/OGF.java
Outdated
@@ -0,0 +1,60 @@ | |||
import java.util.Scanner; |
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 like that all variable names are declared with camelCase
src/main/java/OGF.java
Outdated
|
||
|
||
Task[] tasks = new Task[100]; | ||
int numItem = 0; |
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.
unclear variable name
src/main/java/Task.java
Outdated
if (this.isDone){ | ||
return("[X] " + this.taskName); | ||
} | ||
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.
should be in one line: } else {
src/main/java/OGF.java
Outdated
int taskNo; | ||
while (true) { | ||
String input = scanner.nextLine(); | ||
switch (input.split(" ")[0]) { |
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.
magic number: [0]?
src/main/java/OGF.java
Outdated
System.out.println("Alright, adding this task to the list: "); | ||
System.out.println(tasks[numItem]); | ||
numItem++; | ||
System.out.printf("You have %d tasks in the list.%n", numItem); |
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 use println instead
src/main/java/Task.java
Outdated
@@ -0,0 +1,38 @@ | |||
public class Task { | |||
protected String taskName; |
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.
unclear variable name: does taskName refer to the description, the type of the task, or the task object itself?
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 clean code and very rich features! Good Job!
src/main/java/Event.java
Outdated
@@ -0,0 +1,20 @@ | |||
public class Event extends Task { |
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.
A better name to reflect the inheritance might be EventTask
src/main/java/Deadline.java
Outdated
@@ -0,0 +1,17 @@ | |||
public class Deadline extends Task{ |
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.
A better name to reflect the inheritance relationship might be DeadlineTask
src/main/java/OGF.java
Outdated
System.out.println("Alright, adding this task to the list: "); | ||
System.out.println(tasks[numItem]); | ||
numItem++; | ||
System.out.printf("You have %d tasks in the list.%n", numItem); |
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.
It's best to not repeat yourself, and put these commands into a separate method.
src/main/java/Task.java
Outdated
return taskName; | ||
} | ||
|
||
public boolean getDone(){ |
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.
checkIsDone may be a better name to reflect the boolean nature of the variable to get
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.
Great job so far, but there are still a few improvements to be made
src/main/java/Deadline.java
Outdated
super(description); | ||
this.deadlineString = deadline; | ||
} | ||
@Override |
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 leave a line between methods for improved readability
src/main/java/OGF.java
Outdated
Task[] tasks = new Task[100]; | ||
int numItem = 0; | ||
Scanner scanner = new Scanner(System.in); | ||
int taskNo; |
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 name it taskNum to make it more clear
src/main/java/OGF.java
Outdated
case ("unmark"): | ||
taskNo = Integer.parseInt(input.split(" ")[1])-1; | ||
|
||
if (input.split(" ")[0].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.
perhaps set your mark case as its own case rather than a fallthrough and processing it within your unmark case. helps with readability as well
src/main/java/OGF.java
Outdated
|
||
|
||
Task[] tasks = new Task[100]; | ||
int numItem = 0; |
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 be interpreted as item number or number of items which could be confusing
src/main/java/Deadline.java
Outdated
@@ -0,0 +1,17 @@ | |||
public class Deadline extends Task{ |
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.
Comments are missing for the entire code. Good to have them for better readability.
src/main/java/Event.java
Outdated
private final String startTime; | ||
private final String endTime; |
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 with the usage of Access Modifiers.
src/main/java/OGF.java
Outdated
return (false); | ||
case ("list"): | ||
System.out.println("Here are your tasks for today: "); | ||
for (int i = 0; i < numItem; 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.
Avoid usage of Magic Numbers and literals.
src/main/java/OGF.java
Outdated
printBreakLine(); | ||
} | ||
|
||
private static boolean handleInput (String input) throws OGFException{ |
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.
Very long method. Avoid long methods and take corrective action if a method exceeds 30 lines of code.
src/main/java/OGF.java
Outdated
printBreakLine(); | ||
break; | ||
case ("todo"): | ||
if (!input.contains(" ") || input.indexOf(" ") == input.length()-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.
This is a complex statement, so try to break it down into two boolean variables which handle each sub-expression.
No description provided.