Skip to content
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

[Zeng Yu Ting] iP #227

Open
wants to merge 98 commits into
base: master
Choose a base branch
from
Open

[Zeng Yu Ting] iP #227

wants to merge 98 commits into from

Conversation

Jillzyt
Copy link

@Jillzyt Jillzyt commented Aug 23, 2020

Transform Duke into the Best2103/TBot (Week 4)

Description:

More features are added to Duke. Duke has been transformed into a management platform for tasks. No additional dependencies are required for this change.

Best2103/TBot helps users to keep track of tasks such as todos, deadlines, events. The users can add, delete new tasks and complete tasks. They can also list their tasks to view the existing tasks.

Features added

  1. Add Tasks (Deadline, Todo, Events type)
  2. Add customized messages
  3. Delete Tasks
  4. List all Tasks
  5. Complete Tasks

Fixes # (issue)

None.

Summary of changes

  • [ ✅ ] New feature (non-breaking change which adds functionality)
  • [ ✅ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ✅ ] This change requires a documentation update

How Has This Been Tested?

Currently, there is only one test which is Storage Test. Set up Junit and you will be able to run the StorageTest file.

  • [ ✅ ] Test A test.java.StorageTest

Checklist:

  • [ ✅ ] My code follows the style guidelines of this project
  • [ ✅ ] I have performed a self-review of my own code
  • [ ✅ ] I have commented my code, particularly in hard-to-understand areas
  • [ ✅ ] I have made corresponding changes to the documentation
  • [ ✅ ] My changes generate no new warnings
  • [ ✅ ] I have added tests that prove my fix is effective or that my feature works
  • [ ✅ ] Any dependent changes have been merged and published in downstream modules

j-lum and others added 30 commits August 14, 2019 15:30
Remove references to DukeStub
Change indentation in FXML samples to match AB-3
The OpenJFX plugin expects applications to be modular and bundled
with jlink, resulting in fat jars that are not cross-platform. Let's
manually include the required dependencies so that shadow can package
them properly.
JavaFX tutorial: Support cross-platform JARs
Remove extra '@OverRide' at the start of the code example.
Let's remove generic tutorials from this repo as they are not specific
to this project tempalte
Level-1
Level-1 + Documentation
Level-2
Added Java Documentations with Level 3
Added more java documentations.
Added Documentations
Level-4 Todo and Deadline
Added documentations
Add A-TextUiTesting
Add A-TextUiTesting test cases
Level-5
Level-6
Copy link

@petrickjerico petrickjerico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, Jillzyt! After reviewing your PR, I found that your code has no significant JDoc issues and is generally readable! Provided some possible optimizations that could further improve the code's readability – kindly give them some considerations. Otherwise, your Duke looks neat!

*
*/
public class DukeException extends Exception {
private String errorMesage;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps errorMessage can also be final?

Rationale: At any point, the code would throw new DukeException(errorMessage). Making the attribute(s) final is not only safe, but also recommended by JDoc as a good practice.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted. :) Thank you.

src/main/java/duke/data/Duke.java Outdated Show resolved Hide resolved
src/main/java/duke/storage/DukeDecoder.java Outdated Show resolved Hide resolved
Comment on lines +58 to +60
List<Task> filteredList = internalList
.stream()
.filter(task -> task.description.contains(searchString)).collect(Collectors.toList());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the usage of Streams here to filter relevant tasks!

Noticed that the method means to return UnmodifiableList. Would like to raise the use of Collectors.toUnmodifiableList() as a possible minor optimization.

Reference: https://stackoverflow.com/a/53013505

// Message to add
public static final String MESSAGE_USAGE = COMMAND_WORD + ": Find the tasks that contains the words. \n"
+ "Parameters: find WORD\n" + "Example: " + COMMAND_WORD + " 1";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your code is quite clean in general, but do note that javadocs for all public classes and methods is required.

/**
* Signals that some given data does not fulfill some constraints.
*/
public class IllegalValueException extends DukeException {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps for classes like these that extend off dukeException, we could add additional information about the error itself as compared to having the very generic dukeException error.
This could make the addition of a IllegalValueException have some additional value add.

* Gets the string to be printed for the deadline.
*/
public String toString() {
DateTimeFormatter dateFormat = DateTimeFormatter.ofPattern("MMM dd yyyy HH:mm");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This datetimeformatter could be made into a static method to reduce magic variables



/**
* DukeDencoder decodes all the {@code Task} in the {@code toSave} into a list of decodable

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice use of pointers to other parts of the code! I did not know this existed. Thanks for showing this

Copy link

@blackonyyx blackonyyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, the code was well documented and well done. Good emphasis on the use of some functionalities of jdocs which I will certainly use in my own code as well. Good job!

Fix test files
Add assertions
Edit comments and variable names
Merge branch-A-CodeQuality into master
Add update tasks function
Add update tasks function
Update the user interface
Add credit
Add Ui image
Add a user guide
Update README.md
Update README.md
Rename README.md to INDEX.md
Rename index.md to README.md
Update README.md
Remove documentation files
Update images file path
Edit customized messages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants