-
Notifications
You must be signed in to change notification settings - Fork 193
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
[Huang Che Yen] iP #180
base: master
Are you sure you want to change the base?
[Huang Che Yen] iP #180
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, code looks good. There are only several coding standard violations.
src/main/java/Duke.java
Outdated
public class Duke { | ||
|
||
private static boolean diff_string_check(String[] arr, String toCheckValue) | ||
{ |
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 seems to be a violation of the coding standard, more specifically consider using K&R style brackets. I noticed the same issue in several places below too.
src/main/java/Duke.java
Outdated
@@ -1,10 +1,85 @@ | |||
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.
Unused import. Consider removing this line, it would make the code cleaner.
src/main/java/Duke.java
Outdated
{ | ||
for (int i = 0; i < total_tasks; i += 1) | ||
{ | ||
System.out.print((i+1) + "." + "[" + schedule[i].getStatusIcon() + "] "); |
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.
+
is not followed and preceded by whitespace. This is a violation of the coding standard, more specifically the White space within a statement
guideline.
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.
Code looks good so far. One improvement to be consider is to create subclasses of Task
. For example, adding Todo
, Deadline
, and Event
classes to improve the feature of the app.
src/main/java/Task.java
Outdated
this.isDone = false; | ||
} | ||
|
||
public void markAsDone() |
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 writing a JavaDoc
comment for public methods, as it is meant to be used by others. This seems to be a violation of this coding standard, more specifically it is advisable to write descriptive header comments for all public classes/methods.
src/main/java/Duke.java
Outdated
|
||
private static boolean diff_string_check(String[] arr, String toCheckValue) | ||
{ | ||
boolean test = 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.
You might want to name a boolean variable with something that starts with is or has? It will then follow the coding standards.
src/main/java/Duke.java
Outdated
if (toCheckValue.equals(arr[i])) | ||
{ | ||
test = false; | ||
break; | ||
} | ||
else if (arr[i] == null) | ||
{ | ||
break; | ||
} | ||
} |
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 this violates the coding standards. You can write the code as follows:
if(){
--
} else if(){
} else{
--
}
src/main/java/Task.java
Outdated
public String getStatusIcon() | ||
{ | ||
return (isDone ? "X" : " "); // mark done task with X | ||
} |
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 first "{" of each method should be next to the name of method.
eg. public String getStatusIcon() {
......
}
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 try, consider following the coding standard a little more and SLAP more
src/main/java/Duke.java
Outdated
{ | ||
boolean test = true; | ||
|
||
for (int i = 0; i < arr.length; 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.
Follow Egyptian style brackets.
src/main/java/Duke.java
Outdated
} | ||
|
||
//Done | ||
else if ((line.length() > 4) && line.substring(0, 4).equals("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.
Avoid complicated expressions. Consider splitting the complicated expressions into steps. or use SLAP to extract the logic into separate functions.
src/main/java/Duke.java
Outdated
//Done | ||
else if ((line.length() > 4) && line.substring(0, 4).equals("done")) | ||
{ | ||
int number = Character.getNumericValue(line.charAt(5)); |
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 a more meaningful name than number
which can better describe the value.
Also avoid the use of magic literals. Using a suitable variable name for the constant numbers (which describes the value) would help the understanding of the program.
src/main/java/Duke.java
Outdated
schedule[number - 1].markAsDone(); | ||
} | ||
|
||
// Check for duplicates and add accordingly |
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 removing the comment since the code seems self-explanatory.
src/main/java/Duke.java
Outdated
// Check for duplicates and add accordingly | ||
else if (diff_string_check(task_list, line)) | ||
{ | ||
Task t = new Task(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.
Consider using a more suitable name (noun) instead of an alphabet for the variable.
src/main/java/Duke.java
Outdated
{ | ||
Task t = new Task(line); | ||
schedule[total_tasks] = t; | ||
task_list[total_tasks] = 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.
Variable names must be in camelCase.
src/main/java/Duke.java
Outdated
} | ||
|
||
// Check for duplicates and add accordingly | ||
else if (diff_string_check(task_list, 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.
Names representing methods must be verbs and written in camelCase.
No description provided.