Add FilterCommand #109
Add FilterCommand #109
Conversation
✅ Deploy Preview for shiny-daffodil-45f25f ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov Report
@@ Coverage Diff @@
## master #109 +/- ##
============================================
+ Coverage 68.92% 69.17% +0.24%
- Complexity 509 541 +32
============================================
Files 85 88 +3
Lines 1802 1930 +128
Branches 179 209 +30
============================================
+ Hits 1242 1335 +93
- Misses 505 529 +24
- Partials 55 66 +11
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Amazing implementation of the filter function 💯 Just a few minor changes needed 😄
If you would like to take a look, this is where I refer to for the javadoc coding conventions https://se-education.org/guides/conventions/java/intermediate.html#comments 😊
| } | ||
|
|
||
| /** | ||
| * Checks if the applicant exists. |
There was a problem hiding this comment.
Since the method is comparing between two filter predicates, I think it will be more accurate to state that it checks whether two filter predicates are the same 😸
|
|
||
| /** | ||
| * Checks if the applicant exists. | ||
| * @param other Other applicant. |
There was a problem hiding this comment.
Perhaps the param other should refer to other filter command and not other applicant since we are comparing the predicate of 2 filter commands 🐶
There was a problem hiding this comment.
my bad must have copied the javadoc from another equals command and forgot to change it🤦♂️
| } | ||
|
|
||
| FilterCommand otherFilterCommand = (FilterCommand) other; | ||
| System.out.println("predicate = " + predicate); |
There was a problem hiding this comment.
Is this intended? I think it will be better to remove it if it isn't needed 😄
| /** | ||
| * Parses input arguments and creates a new FilterCommand object | ||
| */ | ||
|
|
There was a problem hiding this comment.
For better coding standard, there should be no blank line between the documentation block and the method/class 😃
|
|
||
| /** | ||
| * Parses a {@code String status} into a {@code Status}. | ||
| * @param status String representation of Status |
There was a problem hiding this comment.
To improve coding standard, can add an empty line between javadoc description and parameter section 👍
|
|
||
| <puml src="diagrams/FilterCommandActivityDiagram.puml" alt="FilterCommandActivityDiagram" /> | ||
|
|
||
| ### Design considerations |
There was a problem hiding this comment.
Goodjob considering the possible alternatives and weighing the pros and cons of each alternative 💯
docs/DeveloperGuide.md
Outdated
|
|
||
| 1. _{ more test cases … }_ | ||
|
|
||
| ### Saving data | ||
|
|
||
| 1. Dealing with missing/corrupted data files | ||
|
|
||
| 1. _{explain how to simulate a missing/corrupted file, and the expected behavior}_ | ||
| 1. _{explain how to simulate a missing/corrupted file, and the expected behavior}_ | ||
|
|
There was a problem hiding this comment.
Thanks for helping to improve the readability of our developer guide by breaking lines that are too long and ensuring consistency throughout the doc 🥳
| /** | ||
| * Returns true if the {@code sentence} contains the {@code word}. | ||
| * Ignores case, but a full word match is required. | ||
| * <br>examples:<pre> |
There was a problem hiding this comment.
Perhaps it is better to remove the html tags from the javadoc? 😄
There was a problem hiding this comment.
i think that the html tags are there to allow the javadoc to be rendered specifically like in this style below. Normally I won't do this, but am following the convention used in this file from #containsWordIgnoreCase
| * containsWordIgnoreCase("ABc def", "DEF") == true | ||
| * containsWordIgnoreCase("ABc def", "AB") == false //not a full word match | ||
| * </pre> | ||
| * @param sentence cannot be null |
There was a problem hiding this comment.
I feel that it will be better to describe what the param is instead of what the param cannot be 😸
| } | ||
|
|
||
| /** | ||
| * @param applicant the input argument |
There was a problem hiding this comment.
I feel that "the input argument" doesn't seem to add much value here, perhaps better to change to "the applicant to be tested"? 🤔
|
Thank you for the thorough review Celestine! I hope I've addressed all your comments 🤠 |
closes #28