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

Search feature implementation #100

Merged

Conversation

shaokiat
Copy link

@shaokiat shaokiat commented Oct 22, 2020

Search Feature:

  • Add ignoreCase method to StringUtil class to convert all arguments into all caps
  • Add SearchCommand
  • Add SearchCommandParser
  • Add ModuleInfoSearcher

SearchCommand:
Uses ModuleInfoSearcher to perform search for ModuleInfo of the module being searched for.
Returns the string format of the module information to be displayed in the Command Line Interface.

SearchCommandParser:
Parse arguments and uses ignoreCase method of StringUtil to convert all arguments into proper moduleCode format before creating a SearchCommand.

ModuleInfoSearcher:
Contains logic to use NusmodsDataManager API and retrieve ModuleInfo from NUSMods or local saved file.

@shaokiat shaokiat self-assigned this Oct 22, 2020
@shaokiat shaokiat added this to the v1.3 milestone Oct 22, 2020
@shaokiat shaokiat linked an issue Oct 22, 2020 that may be closed by this pull request
@shaokiat shaokiat marked this pull request as ready for review October 22, 2020 15:46
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! Just some minor suggestions.

Comment on lines 25 to 27
String moduleCode = StringUtil.ignoreCase(trimmedArgs);

return new SearchCommand(moduleCode);

Choose a reason for hiding this comment

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

Maybe we can catch the possible IllegalArgumentException error thrown by the StringUtil.ignoreCase() method here so that we can display an error message to the user that something has gone wrong.

Currently if the user types in "search MA 1521", the IllegalArgumentException error is thrown in IntelliJ but not in the GUI. Maybe we can try something like that, but I'm not too sure what error message you would wanna show them hehe

Suggested change
String moduleCode = StringUtil.ignoreCase(trimmedArgs);
return new SearchCommand(moduleCode);
try {
String moduleCode = StringUtil.ignoreCase(trimmedArgs);
return new SearchCommand(moduleCode);
}
catch (IllegalArgumentException e) {
throw new ParseException(String.format(MESSAGE_INVALID_COMMAND_FORMAT, SearchCommand.MESSAGE_USAGE));
}

Copy link
Author

Choose a reason for hiding this comment

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

Oh wow good catch! Added the relevant code!

@codecov-io
Copy link

codecov-io commented Oct 23, 2020

Codecov Report

Merging #100 into master will increase coverage by 0.39%.
The diff coverage is 86.04%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #100      +/-   ##
============================================
+ Coverage     72.58%   72.98%   +0.39%     
- Complexity      464      479      +15     
============================================
  Files            80       83       +3     
  Lines          1419     1462      +43     
  Branches        146      151       +5     
============================================
+ Hits           1030     1067      +37     
- Misses          312      317       +5     
- Partials         77       78       +1     
Impacted Files Coverage Δ Complexity Δ
...java/seedu/address/logic/parser/GradPadParser.java 77.27% <0.00%> (-3.68%) 13.00 <0.00> (ø)
...eedu/address/logic/parser/SearchCommandParser.java 77.77% <77.77%> (ø) 3.00 <3.00> (?)
...n/java/seedu/address/logic/ModuleInfoSearcher.java 84.61% <84.61%> (ø) 4.00 <4.00> (?)
...va/seedu/address/logic/commands/SearchCommand.java 93.75% <93.75%> (ø) 5.00 <5.00> (?)
...in/java/seedu/address/commons/util/StringUtil.java 95.23% <100.00%> (+1.12%) 10.00 <3.00> (+3.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 4f7d1aa...9675095. Read the comment docs.

# Conflicts:
#	src/main/java/seedu/address/logic/parser/GradPadParser.java
# Conflicts:
#	src/main/java/seedu/address/logic/parser/GradPadParser.java
@mhdsyfq77
Copy link

LRGTM! super well done 👍🏽 👍🏽

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.

Really really clean code bro 💯 Just a few really really minor comments :D

And although you asked about how to improve your testing, I actually think your testing is damn efficient man. Test coverage for the code you've written is almost 100% :O The only test gap is the test for the searchCommand branch in GradPadParser 😁

@mhdsyfq77 mhdsyfq77 merged commit adf000a into AY2021S1-CS2103T-T09-1:master Oct 23, 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.

Implement Search Command
5 participants