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

[WIP] Increase maintainability #121

Closed

Conversation

mmachatschek
Copy link
Contributor

Purpose

Partly fixes maintainability part of #114

Approach

Extract methods of Item class into separate traits

Open Questions and Pre-Merge TODOs

  • composer lint and composer fix was executed.
  • Tests were written and pass with 100% coverage.
  • A issue with a detailed explanation of the problem/enhancement was created and linked.

@mmachatschek
Copy link
Contributor Author

mmachatschek commented Mar 8, 2020

@howard looks like there is some issue with the branch policies. I don't know if it's fixed with this but heres a link to a issue at github.community

@mmachatschek mmachatschek changed the title Increase maintainability [WIP] Increase maintainability Mar 8, 2020
@howard
Copy link
Collaborator

howard commented Mar 8, 2020

@mmachatschek It seems that it's intentional behavior with GitHub Actions that they are not executed on PRs originating from a forked repo for security reasons, e.g. so that a bad actor can't exfiltrate stored secrets. Not sure how to solve this for external contributions like this. You might have to set up those actions in your fork, but I'm not sure if that will help.

Alternatively, you can start working directly on this repo - I just gave you write access.

@howard
Copy link
Collaborator

howard commented Mar 8, 2020

@mmachatschek Concerning the approach you took to placate CodeClimate, I admire your efforts, but I'm torn about whether it's the right way to do this. Some thoughts:

  • Note sure it traits are the right constract to split this up. Perhaps some of the more complex things, like attributes, would do better with separate container classes having all the logic, and Item only exposing operations to fill this container.
  • Item used to be a POPO, and then validation logic crept into it. Perhaps those checks can be extracted in a meaningful way.
  • As for the complaint that the number of methods is too large, one could argue that (assuming that Item pretty much becomes a POPO again) a maintainability check that counts methods is "dumb", because the number of methods doesn't correlate as strongly with complexity if they are largely devoid of logic. At least for this, it would make sense to add an exception to the rule.
  • Traits could make sense to extract commonly used validations, e.g. checks for empty values.
  • As for file structure, traits are just another construct like classes and interfaces, so I don't think they should be segregated in a separate namespace based on which construct they are, but placed in the context where they make sense. For example, if I have traits for commonly done validations, a good spot for them is probably \FINDOLOGIC\Export\Data\Validations, because the validations concern the stuff in ...\Data, not globally, but are not logically on the same level as the stuff in ...\Data.

I don't have the answer how to do this right - it will take some thinking to get there.

@mmachatschek
Copy link
Contributor Author

@howard thanks for the input. I will gladly help increasing the maintainability to Level A as soon as you have a direction into which we want to go for version 3.0. Just bump me in the corresponding issue 👍

@mmachatschek mmachatschek deleted the increase_maintainability branch March 31, 2020 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants