-
Notifications
You must be signed in to change notification settings - Fork 434
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
[HusseinSafwan02] iP #463
base: master
Are you sure you want to change the base?
[HusseinSafwan02] iP #463
Conversation
Let's tweak the docs/README.md (which is used as the user guide) to fit Duke better. Specifically, 1. mention product name in the title 2. mention adding a product screenshot and a product intro 3. tweak the flow to describe feature-by-feature
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.
LGTM, just a tiny issue of unnecessary spaces at different places in the code.
src/main/java/Duke.java
Outdated
public class Duke { | ||
|
||
// confirmation comment after adding a task | ||
public static String addComment (Task task, int 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.
Shouln't there be no space following addComment
and the arguments it takes in?
src/main/java/Duke.java
Outdated
return message; | ||
} | ||
|
||
public static String introMessage() { |
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 another name for the method? Something like getIntroMessage()
?, as introMessage()
sounds like a String variable instead 😅
src/main/java/Duke.java
Outdated
return greeting + mickey + intro; | ||
} | ||
|
||
public static String outroMessage() { |
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.
Please refer to the comment given for introMessage()
src/main/java/Deadline.java
Outdated
@@ -0,0 +1,16 @@ | |||
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.
There should be a space after Task
and {
right? I noticed the same issue in other places, so just mind that :)
src/main/java/FileManager.java
Outdated
|
||
private Task parseTask(String data) { | ||
String[] parts = data.split(" \\| "); | ||
if (parts.length < 3) return null; // Basic validation |
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 use the K&R style brackets (Egyptian style) brackets here for better readability?
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, your code is easy to understand. Most of your code follows the coding standard. However, there are some minor things you may fix. In all, LGTM, good job!
src/main/java/Duke.java
Outdated
return greeting + mickey + intro; | ||
} | ||
|
||
public static String outroMessage() { |
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 it's better to make the method name more understandable.
src/main/java/Duke.java
Outdated
System.out.println(addComment(tasks.get(count - 1), count)); | ||
fileManager.saveTasks(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.
Not sure if this empty line is left on purpose or not.
src/main/java/Event.java
Outdated
return this.time; | ||
} | ||
|
||
@Override public String toString() { |
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.
Better to put Override above the method.
src/main/java/Duke.java
Outdated
public static void listTasks(ArrayList<Task> taskList, int count) { | ||
System.out.print("____________________________________________________________\r\n"); | ||
System.out.println(" Here are the tasks in your list:"); | ||
for (int i = 0; i < taskList.size(); 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.
Better to delete space after ++
src/main/java/Duke.java
Outdated
return message; | ||
} | ||
|
||
public static String replacer(String input) { |
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.
Better to use a verb for method naming.
src/main/java/Duke.java
Outdated
|
||
|
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.
better not to leave so many empty lines.
src/main/java/Duke.java
Outdated
if (isEvent) { | ||
String task = getTask(userInput); | ||
int index = userInput.indexOf("/"); | ||
String when = replacer(userInput).substring(index); |
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.
Better use a noun to name variable.
src/main/java/Duke.java
Outdated
} | ||
if (isDeadline) { | ||
int index = userInput.indexOf("/"); | ||
String when = userInput.substring(index + 3); |
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.
same as above
src/main/java/FileManager.java
Outdated
} | ||
|
||
private Task parseTask(String data) { | ||
String[] parts = data.split(" \\| "); |
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 rename parts to something more understandable?
# Conflicts: # src/main/java/chillchief/ChillChief.java
A 'help' command is useful for new users who are using ChillChief for the first time. Let's, * update the parser to handle the 'help' command * update the TextUi to generate the text Ui for the user
No checking of important assumptions at various points in code. Checking of assumptions through assertions ensures that different points of the code is functioning as expected. Assertions added at key points with assumptions in private methods. Ensures arguments for private methods are valid.
Improved Code Quality
Assertions for important assumptions.
ChillChief
ChillChief is your go to assistant 'bro' for managing tasks in your life. It is
All 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: