[WIP] Increase maintainability

mmachatschek commented 4 years ago

Purpose

Partly fixes maintainability part of #114

Approach

Extract methods of Item class into separate traits

Open Questions and Pre-Merge TODOs

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

mmachatschek commented 4 years ago

@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

howard commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 :+1:

findologic / libflexport