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

Update FindCommand to handle prefixes #68

Closed

Conversation

yhanyi
Copy link

@yhanyi yhanyi commented Oct 13, 2024

Code review, potentially fixes #60

@yhanyi yhanyi added the priority.Medium Nice to have label Oct 13, 2024
@yhanyi yhanyi added this to the v1.3 milestone Oct 13, 2024
@yhanyi yhanyi self-assigned this Oct 13, 2024
Copy link

codecov bot commented Oct 13, 2024

Codecov Report

Attention: Patch coverage is 29.62963% with 19 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...model/student/CourseContainsKeywordsPredicate.java 0.00% 10 Missing ⚠️
.../seedu/address/logic/parser/FindCommandParser.java 25.00% 6 Missing and 3 partials ⚠️
Files with missing lines Coverage Δ Complexity Δ
...java/seedu/address/logic/commands/FindCommand.java 100.00% <100.00%> (+18.75%) 6.00 <5.00> (+1.00)
...s/model/student/NameContainsKeywordsPredicate.java 100.00% <100.00%> (ø) 8.00 <4.00> (+1.00)
.../seedu/address/logic/parser/FindCommandParser.java 30.76% <25.00%> (-47.01%) 3.00 <2.00> (-1.00)
...model/student/CourseContainsKeywordsPredicate.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)

... and 5 files with indirect coverage changes

Copy link

@notnotmax notnotmax left a comment

Choose a reason for hiding this comment

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

Good work so far, just some issues to bring up regarding the course predicate.

import seedu.address.commons.util.StringUtil;

/**
* Tests that a {@code Course}'s {@code courseCode} matches any of the keywords given.

Choose a reason for hiding this comment

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

I think we want course matching to be exact and case-insensitive, because a tutor for CS2040 may want to exclude students from CS2040S for example.

Choose a reason for hiding this comment

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

I've worked on this, see #71.

public boolean test(Student student) {
return keywords.stream()
.anyMatch(keyword ->
student.getCourses().stream()

Choose a reason for hiding this comment

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

I understand this works, but maybe for the sake of readability / slap / less nesting etc, maybe a separate function to get a student's courses as a List<String> could be used? (Then we can do .anyMatch here.)

Choose a reason for hiding this comment

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

Logic for this looks good.

@notnotmax
Copy link

This PR is outdated. The issues it aimed to fix have been covered by #83.

@notnotmax notnotmax closed this Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority.Medium Nice to have
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change FindCommand to return intersection on multiple args
2 participants