-
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
[Hwww23] iP #467
base: master
Are you sure you want to change the base?
[Hwww23] iP #467
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
Update Duke.java Update Event.java Update Todo.java
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 found your code easy to follow for the most part except for logic in Duke.java
as it is quite long. You can consider splitting the class into smaller classes if that makes sense. I noted a few minor coding standard violations too. Press on! 👍
src/main/java/Duke.java
Outdated
import java.io.*; | ||
import java.util.*; |
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.
Following our coding standard, imported classes should be listed explicitly. For example,
import java.io.*; | |
import java.util.*; | |
import java.io.BufferedReader; |
I noticed the same issue in several other places as well.
src/main/java/Duke.java
Outdated
switch (type) { | ||
case "T": | ||
s = loadInput[2].trim(); | ||
task = new Todo(s); | ||
|
||
if (done.equals("1")) { | ||
task.markAsDone(); | ||
} | ||
|
||
myList.addItem(task); | ||
break; | ||
case "D": | ||
s = loadInput[2].trim(); | ||
s1 = loadInput[3].trim(); | ||
task = new Deadline(s, s1); | ||
|
||
if (done.equals("1")) { | ||
task.markAsDone(); | ||
} | ||
|
||
myList.addItem(task); | ||
break; | ||
case "E": | ||
s = loadInput[2].trim(); | ||
s1 = loadInput[3].trim(); | ||
String s2 = loadInput[4].trim(); | ||
task = new Event(s, s1, s2); | ||
|
||
if (done.equals("1")) { | ||
task.markAsDone(); | ||
} | ||
|
||
myList.addItem(task); | ||
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.
Following our coding standards, case
clauses should not have indentation in switch statements.
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.
Should the logic in this method be separated instead? Perhaps you can consider using a getter method for the boolean variable isDone
and format the output based on the variable in the toString()
method.
src/main/java/Event.java
Outdated
protected String start; | ||
protected String end; |
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.
Any reason why you decided to set the access modifier of these two variables to protected
instead of private
?
src/main/java/MyList.java
Outdated
public class MyList { | ||
private List<Task> items; | ||
|
||
public MyList() { | ||
this.items = new ArrayList<>(); | ||
} | ||
|
||
public String addItem(Task t) { | ||
this.items.add(t); | ||
return "Got it. I've added this task:\n" + t.toString() + "\nNow you have " + this.items.size() + " tasks in the list."; | ||
} | ||
|
||
public String getItems() { | ||
StringBuilder stringBuilder = new StringBuilder(); | ||
stringBuilder.append("Here are the tasks in your list:\n"); | ||
int index = 1; | ||
|
||
for (Task task : this.items) { | ||
String itemString = String.format("%d. %s\n", index, task.toString()); | ||
stringBuilder.append(itemString); | ||
index++; | ||
} | ||
|
||
return stringBuilder.toString(); | ||
} | ||
|
||
public String markTask(int index) { | ||
if (index < 1 || index > this.items.size()) { | ||
return "Number is outside length of list"; | ||
} else { | ||
return "Nice! I've marked this task as done:\n" + this.items.get(index - 1).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.
I like your naming of methods adhere to the coding standard 👍. One small nit would be perhaps to choose between task
or item
in your method names in this class
Update Deadline.java Update Event.java Update duke.txt
Update Duke.java, MyList.java, Task.java, Todo.java, Deadline.java, Event.java
Update build.gradle
src/main/java/duke/MyList.java
Outdated
} | ||
|
||
public MyList(List<Task> t) { | ||
this.items = 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.
I think this
can be omitted from the line
src/main/java/duke/Parser.java
Outdated
Request request = getRequest(userInput); | ||
|
||
switch (request) { | ||
case 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.
I think this should be deindented to follow our switch case convention.
src/main/java/duke/Event.java
Outdated
public String toString() { | ||
DateTimeFormatter customFormatter = DateTimeFormatter.ofPattern("MMM d yyyy h:mma"); | ||
String formattedDateTime = this.start.format(customFormatter); | ||
String formattedDateTime1 = this.end.format(customFormatter); |
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 change formattedDateTime1
to something more descriptive, like formattedEndDateTime
src/main/java/duke/Storage.java
Outdated
String taskString, byString, dateTimePattern, fromString, toString; | ||
Task task; | ||
|
||
switch (type) { |
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.
Similar comment as above regarding switch-case indentation.
src/test/java/duke/StorageTest.java
Outdated
public class StorageTest { | ||
|
||
@Test | ||
public void testSaveAndLoad() throws DukeException, IOException { |
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 don't think this follows our coding standard for naming tests, it should follow the format: featureUnderTest_testScenario_expectedBehavior()
src/test/java/duke/ParserTest.java
Outdated
public class ParserTest { | ||
|
||
@Test | ||
public void testGetRequest() { |
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 don't think this follows our coding standard for naming tests, it should follow the format: featureUnderTest_testScenario_expectedBehavior()
src/main/java/duke/MyList.java
Outdated
|
||
public String markTask(int index) { | ||
if (index < 1 || index > this.items.size()) { | ||
return "Number is outside length of 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.
Would it have been better to throw an exception instead, since this is an error in user input?
src/main/java/duke/MyList.java
Outdated
|
||
public String delete(int index) { | ||
if (index < 1 || index > this.items.size()) { | ||
return "Number is outside length of 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.
Same for here
…ava, ParserTest.java and StorageTest.java To adhere to java coding standards.
To implement find function
To resolve a bug with saving task
Added assertions to addTask method in MyList.java and save method in Storage.java. - Ensure that task added into the list should not be null or have an empty description. - Ensure that the list being saved into duke.txt is not null.
Update MyList.java, Storage.java
DeleteCommand.java, FindCommand.java, InvalidCommand.java, ListCommand.java and Mark.java and update Ui.java, Storage.java, Parser.java and Duke.java There were methods that were too long which caused deep nesting in Storage.java and Parser.java. The Command class was added to reduce the length of methods by making parser return a specific child class of the Command class, the required operation is then executed depending on the command class.
Conflicts for different method parameter names.
Add Command class
update Parser.java, MyList.java, Event.java and Deadline.java A requirement to provide support for recurring tasks. Provide a RecurringTask interface for deadlines and events and and implement a scheduler so date time can update automatically
Rename chatbot name
Update user guide
update name of jar file
Storage.java Whether a task was recurring a not was not saved and loaded properly. Implement conditions in Storage.java to load recurring task correctly and made some changes to the save() method in Deadline.java and Event.java for proper saving.
Fix invalid datetime bug, e.g. 2022-03-33 2500 and invalid userinput, e.g. todoabc
PepegPro
DukePro frees your mind from having to remember things you need to do. It's,
FASTSUPER FAST to useAll you need to do is,
And it is FREE!
Features: