-
Notifications
You must be signed in to change notification settings - Fork 362
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[nicljr] iP #369
base: master
Are you sure you want to change the base?
[nicljr] iP #369
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.
Learnt a lot from your code! Just some suggestions to improve code quality. Thank you!! :)
src/main/java/duke/Ui.java
Outdated
import java.util.Scanner; | ||
|
||
public class Ui { | ||
public static String HELLO = "Hello! I am Duke Nice To Meet You\n"; |
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 HELLO_MSG be more explanatory for this variable?
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, the code structure looks good! There were a few comments regarding following proper coding standards for naming. However, it is still a very good job done :)
|
||
public TaskAssigner() {} | ||
|
||
public Task assignTask(String command) throws DukeException { |
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 your function name is written beginning with a verb! It helps to convey the action which the function is performing.
src/main/java/duke/TaskAssigner.java
Outdated
} | ||
|
||
public Task assignEvent(String command) throws DukeException { | ||
String[] splitTimings = command.split("/from | /to "); |
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.
May I clarify the intention behind the naming of this variable? Perhaps another name could be choosen to convey the message that the variable is an array of strings representing time (ie. timestampsAsString
)
src/main/java/duke/TaskAssigner.java
Outdated
int s_index = command.indexOf("/from") + 6; | ||
int e_index = command.indexOf("/to") + 4; | ||
String start_date = TimeChecker.updateTime(command.substring(s_index, e_index - 5)); | ||
String end_date = TimeChecker.updateTime(command.substring(e_index)); | ||
String e_desc = command.substring(6, s_index - 6); |
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.
Not sure if I like the naming of these variables, as they do not follow camelCase
.
src/main/java/duke/Ui.java
Outdated
System.out.println(HELLO); | ||
} | ||
|
||
public void 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.
May I clarify the intention behind the naming of this function? Perhaps you could consider printBye()
instead, so as to convey the action performed by the function.
public class Deadline extends Task { | ||
|
||
static protected String DEFAULT_TIME = "2359"; | ||
protected LocalDateTime by; |
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.
Not sure if others will understand what the by
variable represents on first look! Perhaps you could consider renaming it to dateBy
so as to convey to others that it is a date!
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, I think most of your code following the Java coding standard!
But you may want to take note of the few instances where you used snake_case instead of camelCase for naming variables. These issues should be detected by using checkStyle.
Otherwise, the layout and statements seem to follow the coding standard. Good work!
src/main/java/duke/TaskAssigner.java
Outdated
String d_desc = command.substring(9, d_index - 4); | ||
return new Deadline(d_desc, deadline); | ||
return new Deadline(d_desc, updated_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.
Might want to consider changing updated_deadline
to updatedDeadline
to adhere to the Java coding standard of naming variables in camelCase. I noticed that you used snake_case instead of camelCase in several other areas of your code as well. Might I suggest using checkStyle to detect such cases which could save you a lot of time instead of finding them manually.
src/main/java/duke/Parser.java
Outdated
public static String[] deadlineSplit(String deadline) { | ||
return deadline.split(" ", 2); | ||
} | ||
|
||
public static String[] eventSplit(String event) { | ||
return event.split(" ", 6); | ||
} | ||
|
||
public static LocalDateTime stringToDateTime(String duration) { | ||
LocalDateTime localDateTime = LocalDateTime.parse(duration, strFormatter); | ||
return localDateTime; | ||
} | ||
|
||
public static String dateTimeToString(LocalDateTime localDateTime) { |
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 methods such as deadlineSplit
be named something like splitDeadlineIntoStringArray
to better follow the convention of naming methods using verbs?
import java.time.LocalDateTime; | ||
import java.time.format.DateTimeFormatter; | ||
|
||
public class Parser { |
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 Parser have a descriptive header comment?
src/main/java/duke/Storage.java
Outdated
public class Storage { | ||
|
||
public static String path = "DukeData/tasks.txt"; | ||
public static String dirPath = "DukeData"; |
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 this variable be converted to a constant?
src/main/java/duke/Storage.java
Outdated
Task curr = new Task(""); | ||
|
||
switch (text[0]) { | ||
case ("T"): |
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 the indentation for case clauses be removed to follow the coding standard?
case ("T"): | |
switch (text[0]) { | |
case ("T"): |
src/main/java/duke/Ui.java
Outdated
import java.util.ArrayList; | ||
|
||
public class Ui { | ||
public static String GREET_MSG = "Hello! I am Duke Nice To Meet You\n"; |
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.
Nice use of uppercase with underscores for naming the constants!
…an start date. An event with an end date canonot be earlier than the start date, this is to ensure that Duke catches this error and doesn't accept this invalid input. We want to ensure the correctness of our Tasks in Duke. Event constructor will throw a DukeException if such an input arrives, by utilising the LocalDateTime isBefore() method. Event holds the LocalDateTime information, hence one way I thought was to throw the Exception within the Event Class.
This reverts commit 35810a9.
Duke A-Assertions
This reverts commit 00bdbb8.
This reverts commit 3bceb3a.
Coding Standard needs to be adhered for clarity Removed Magic Numbers Ensured no Deep Nesteing, ArrowHead No long method names Ensure clarity of method names Done as suggested by the textbook provided
A-CodingStandard: Fix Coding Standards
…past week and Completed Task within the past week. Provided Insight on User Productivity through checking Urgency of various deadline tasks.
DukePro
DukePro frees your mind of having to remember things you need to do. It's,
FASTSUPER FAST to useAll you need to do is,
And it is FREE!
Features:
If you are a Java programmer, you can use it to practice Java too. Here's the main method: