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

Add match command implementation to developer guide #282

Merged

Conversation

keanecjy
Copy link

No description provided.

@keanecjy keanecjy added this to the v1.3 milestone Oct 26, 2020
@keanecjy keanecjy requested a review from a team October 26, 2020 14:04
@keanecjy keanecjy self-assigned this Oct 26, 2020
@keanecjy keanecjy linked an issue Oct 26, 2020 that may be closed by this pull request

**Extensions**

 2a. User have no internships that matches her profile skills. <br/>

Choose a reason for hiding this comment

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

should you include an extension that nothing is shown?

docs/DeveloperGuide.md Outdated Show resolved Hide resolved
Copy link

@seanjyjy seanjyjy 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 nits to fix

`getInternshipList`, and `getMatchingInternships` are implemented within `MatchCommand`.

- Pros:
- Still adheres to the Single Responsibility Principle as the `MatchCommand` is meant to generate the list of

Choose a reason for hiding this comment

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

you could abstract srp into glossary.

If i recall correctly there is more than one usage of OOP also, perhaps you could help add them into the glossary!

docs/DeveloperGuide.md Outdated Show resolved Hide resolved
Copy link

@seanjyjy seanjyjy left a comment

Choose a reason for hiding this comment

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

LGTM

@seanjyjy seanjyjy merged commit e12b695 into AY2021S1-CS2103T-T15-4:master Oct 26, 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.

Add skill matching diagrams in developer guide
2 participants