-
Notifications
You must be signed in to change notification settings - Fork 252
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
[AY1920S1-CS2113-T16-2] El Duque #35
base: master
Are you sure you want to change the base?
[AY1920S1-CS2113-T16-2] El Duque #35
Conversation
Rework UI for date time input
fix bug with filtered edit command
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.
Take a look at my comments for some suggestions about your code. Keep in mind the Java Coding standards.
I have noticed that most of the commands so far are the Duke commands. I hope you are working on your new features already. Feel free to speak to us if you are facing any difficulty!
|
||
|
||
public AddCommand(String description, String taskType, Optional<String> filter) { | ||
this.taskType = taskType; |
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.
Important that you guys have JavaDoc comments for all classes. You can find the Java Coding Standards on the moduel website
} | ||
if (filter == tempTaskList.get(i).getFilter()) { | ||
filteredListCounter++; | ||
} |
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 the indentation for this if statement correct?
String keyword = fcArray[0]; | ||
|
||
switch (keyword) { | ||
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.
There is no indentation for case clauses in Java. Configure CheckStyle to follow this style
* .create(); | ||
* }</pre> | ||
* Like {@code GsonBuilder}, this API supports chaining: <pre> {@code | ||
* RuntimeTypeAdapterFactory<Shape> shapeAdapterFactory = RuntimeTypeAdapterFactory.of(Shape.class) |
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.
Best to delete commented out code. Makes your code more readable
src/main/java/duke/task/Task.java
Outdated
} | ||
|
||
public void setPriority(int i) throws DukeException { | ||
switch(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.
Could the getting and setting of priority be handled elsewhere? (take note of SoC)
src/test/java/AddCommandTest.java
Outdated
// assertEquals(LocalDate.of(2019, 10, 19), a.convertToLocalDate("19102019 1000")); | ||
// final AddCommand b = new AddCommand("homework", "19012019 1000"); | ||
// assertEquals(LocalDate.of(2019, 01, 19), a.convertToLocalDate("19012019 1000")); | ||
} |
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.
Delete commented out code
src/test/java/EditCommandTest.java
Outdated
public class EditCommandTest { | ||
|
||
// @Test | ||
|
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 would suggest that you guys write some test code, as and when you update the code
Added new print functionality
Optimised Parser, Command, Storage. Deleted Deadline and ToDo classes
Optimised TaskList
Optimised AddCommand
Optimised Event
Optimised Task
Created DateTimeParser
Optimise UI interface
fix check style issues
Update DG and UndoCommandTest
fixed checkstyle
Update Documentation
@shaun97
@nimiew