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 Command implementation in Developer guide #214

Merged

Conversation

keanecjy
Copy link

No description provided.

@keanecjy keanecjy added this to the v1.3 milestone Oct 20, 2020
@keanecjy keanecjy requested review from ZoroarkDarkrai and a team October 20, 2020 13:03
@keanecjy keanecjy self-assigned this Oct 20, 2020
@keanecjy keanecjy linked an issue Oct 20, 2020 that may be closed by this pull request
Copy link

@ZoroarkDarkrai ZoroarkDarkrai left a comment

Choose a reason for hiding this comment

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

Nice work!

- Pros:
- Each command has its own specific task to execute. This means that classes are more flexible and can be changed
very easily.
- Higher cohesion as the class is only dependent on the one `Item` type

Choose a reason for hiding this comment

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

Why does it have higher cohesion?
Isn't low dependency --> low coupling?

Copy link
Author

Choose a reason for hiding this comment

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

Did you misread?
Quoting the textbook:
Cohesion is a measure of how strongly-related and focused the various responsibilities of a component are.

Copy link

Choose a reason for hiding this comment

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

"Coupling is a measure of the degree of dependence between components, classes, methods, etc. Low coupling indicates that a component is less dependent on other components." ?

Copy link
Author

Choose a reason for hiding this comment

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

There's no coupling word used here?

Choose a reason for hiding this comment

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

"Only dependent on the one Item type" --> low dependency --> low coupling?

@keanecjy
Copy link
Author

Generalize commands to use XCommand and YCommand instead

@codecov-io
Copy link

Codecov Report

Merging #214 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #214   +/-   ##
=========================================
  Coverage     48.23%   48.23%           
  Complexity      760      760           
=========================================
  Files           186      186           
  Lines          3477     3477           
  Branches        392      392           
=========================================
  Hits           1677     1677           
  Misses         1694     1694           
  Partials        106      106           

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 88e24fc...f5a5eb6. Read the comment docs.

@ZoroarkDarkrai
Copy link

Nvm I'll just merge this first

@ZoroarkDarkrai ZoroarkDarkrai merged commit edc2f35 into AY2021S1-CS2103T-T15-4: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.

Add command implementation to Developer Guide
3 participants