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

Change find command to allow better searches #77

Merged

Conversation

mhdsyfq77
Copy link

  • Changed StringUtil method to check use contains() rather than equalsIgnoreCase() to improve module(s) search
  • Resolved test failures as a result of change

Minor nit:

  • Uppercase-d moduleCode variable in ModuleCode so that when user enters add c/cs2103t cr/4, the module code displayed in the GUI would be displayed as "CS2103T", for standardisation purposes

Closes #76

@mhdsyfq77 mhdsyfq77 added this to the v1.3 milestone Oct 20, 2020
@mhdsyfq77 mhdsyfq77 requested a review from a team October 20, 2020 10:24
@mhdsyfq77 mhdsyfq77 self-assigned this Oct 20, 2020
@codecov-io
Copy link

codecov-io commented Oct 20, 2020

Codecov Report

Merging #77 into master will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #77      +/-   ##
============================================
- Coverage     74.46%   74.42%   -0.05%     
  Complexity      424      424              
============================================
  Files            75       75              
  Lines          1253     1251       -2     
  Branches        125      125              
============================================
- Hits            933      931       -2     
  Misses          273      273              
  Partials         47       47              
Impacted Files Coverage Δ Complexity Δ
...in/java/seedu/address/commons/util/StringUtil.java 94.11% <100.00%> (-0.62%) 7.00 <3.00> (ø)
...in/java/seedu/address/model/module/ModuleCode.java 80.00% <100.00%> (ø) 6.00 <0.00> (ø)
...el/module/ModuleCodeContainsKeywordsPredicate.java 100.00% <100.00%> (ø) 7.00 <1.00> (ø)

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 c23fe05...28416a8. Read the comment docs.

Copy link

@Silvernitro Silvernitro left a comment

Choose a reason for hiding this comment

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

Just a few comments! But nice work here man, I love the new find ux

@@ -20,22 +19,20 @@
* containsWordIgnoreCase("ABc def", "DEF") == true

Choose a reason for hiding this comment

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

Let's update the javadocs too 😁

* - middle word in sentence
* - matches first word in ModuleCode
* - last word in ModuleCode
* - middle word in ModuleCode

Choose a reason for hiding this comment

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

Should we add "matches part of any word in ModuleCode" too? Since now we support it

* - any word
* - word containing symbols/numbers
* - word with leading/trailing spaces
*
* Valid equivalence partitions for sentence:
* Valid equivalence partitions for ModuleCode:
* - empty string
* - one word
* - multiple words
Copy link

Choose a reason for hiding this comment

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

Do you think we should remove "multiple words" ah? I feel like it might be an irrelevant equivalence class now that we use a .contains() check

Removing it will also match the new test cases below since we're removing the old tests involving multiple words

Choose a reason for hiding this comment

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

yup agree!

Copy link

@yan-soon yan-soon left a comment

Choose a reason for hiding this comment

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

LGTM! (apart from minor comments)

* - any word
* - word containing symbols/numbers
* - word with leading/trailing spaces
*
* Valid equivalence partitions for sentence:
* Valid equivalence partitions for ModuleCode:
* - empty string
* - one word
* - multiple words

Choose a reason for hiding this comment

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

yup agree!

Comment on lines 98 to 99
* - query word matches part of a sentence word
* - sentence word matches part of the query word
* - ModuleCode word matches part of the CharSequence

Choose a reason for hiding this comment

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

Is this part supposed to return true now? now that we changed the find command to .contains()

Copy link

@Silvernitro Silvernitro left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link

@yan-soon yan-soon left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@yan-soon yan-soon left a comment

Choose a reason for hiding this comment

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

LGTM!

@Silvernitro Silvernitro merged commit 4fc17ec into AY2021S1-CS2103T-T09-1:master Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change find command to suit more flexible searches (e.g. CS2...)
5 participants