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

Country filter v1 #121

Merged
merged 31 commits into from
Oct 11, 2020
Merged

Conversation

raysonkoh
Copy link

@raysonkoh raysonkoh commented Oct 9, 2020

Fixes #108
Basic implementation of country filter c/COUNTRY_CODE command. Tested working with GUI on country filter c/SG.

@tankangliang note that when u add the country field to Client, you need to update the client.getCountry() method as I am hardcoding the return country.

Better tests and user feedback messages will be done in a separate issue.

@raysonkoh raysonkoh added type.Story A user story priority.High Must do labels Oct 9, 2020
@raysonkoh raysonkoh added this to the v1.2 milestone Oct 9, 2020
@raysonkoh raysonkoh self-assigned this Oct 9, 2020
@raysonkoh raysonkoh added this to In progress in v1.2 via automation Oct 9, 2020
@codecov-io
Copy link

codecov-io commented Oct 9, 2020

Codecov Report

Merging #121 into master will decrease coverage by 0.82%.
The diff coverage is 65.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #121      +/-   ##
============================================
- Coverage     72.10%   71.28%   -0.83%     
- Complexity      410      440      +30     
============================================
  Files            76       81       +5     
  Lines          1269     1379     +110     
  Branches        127      143      +16     
============================================
+ Hits            915      983      +68     
- Misses          320      345      +25     
- Partials         34       51      +17     
Impacted Files Coverage Δ Complexity Δ
.../seedu/address/logic/parser/AddressBookParser.java 56.41% <0.00%> (-28.78%) 13.00 <0.00> (ø)
...c/main/java/seedu/address/model/client/Client.java 95.23% <0.00%> (-2.33%) 21.00 <0.00> (ø)
...main/java/seedu/address/model/country/Country.java 60.00% <22.22%> (-21.82%) 6.00 <2.00> (+3.00) ⬇️
...u/address/logic/commands/CountryFilterCommand.java 46.15% <46.15%> (ø) 2.00 <2.00> (?)
...ent/ClientCountryMatchesInputCountryPredicate.java 50.00% <50.00%> (ø) 2.00 <2.00> (?)
...edu/address/logic/commands/CountryNoteCommand.java 70.58% <70.58%> (ø) 4.00 <4.00> (?)
src/main/java/seedu/address/model/note/Note.java 75.00% <75.00%> (-25.00%) 2.00 <2.00> (+1.00) ⬇️
...va/seedu/address/model/country/CountryManager.java 85.71% <76.92%> (-14.29%) 11.00 <8.00> (+6.00) ⬇️
...in/java/seedu/address/logic/parser/ParserUtil.java 93.47% <80.00%> (-3.75%) 17.00 <3.00> (+3.00) ⬇️
...eedu/address/logic/commands/ClientFindCommand.java 100.00% <100.00%> (ø) 6.00 <0.00> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 725bbec...1342ba3. Read the comment docs.

@tankangliang tankangliang mentioned this pull request Oct 10, 2020
Copy link
Member

@rtshkmr rtshkmr left a comment

Choose a reason for hiding this comment

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

this PR is required for other PRs to be made, hence it got expedited. Should be okay after the minor changes that @tankangliang suggested. Thanks man!

v1.2 automation moved this from In progress to Review in progress Oct 10, 2020
Copy link

@tankangliang tankangliang left a comment

Choose a reason for hiding this comment

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

The rest of the code looks good!

src/main/java/seedu/address/model/country/Country.java Outdated Show resolved Hide resolved
Comment on lines 56 to 61
public boolean hasCountryNote(Country country, Note countryNote) {
if (!isValidCode(country.getCountryCode())) {
return false;
}
return countryCodeMap.get(country.getCountryCode()).hasCountryNote(countryNote);
}
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm I was thinking that this CountryManager class holds all Country objects, while every other class should be holding references to these objects instead of any new Country() object. That way, the class themselves should be able to check if the Country has any notes, and this method would not be needed.

Copy link
Author

Choose a reason for hiding this comment

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

My idea is that whenever you are interacting with the existing Country instances (other than just getting country notes), you should only do it via CountryManager. I think this layer of abstraction is good especially for logging/debugging in the future since Model only needs to operate on CountryManager and I would be able to log all Country method calls.

For classes like the CountryNoteParser or CountryFilterParser they should not be holding references to the existing Country as they do not have a reference to CountryManager. Only Model has a reference to CountryManager. So when a Command executes on a Model, then they are able to check for duplicate notes, etc.

Also, Country has a hasCountryNote method, but I purposely made it protected so that only CountryManager can call that method. Other classes should only be able to call the constructor and getCountryNote method of Country only.

v1.2 automation moved this from Review in progress to Reviewer approved Oct 11, 2020
@raysonkoh raysonkoh merged commit c574e5c into AY2021S1-CS2103T-F11-4:master Oct 11, 2020
v1.2 automation moved this from Reviewer approved to Done Oct 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority.High Must do type.Story A user story
Projects
No open projects
v1.2
Done
Development

Successfully merging this pull request may close these issues.

Add command for filtering by country
5 participants