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

Customization of evaluation of new child objects #331

Open
estevao-schultz-neto-SAP opened this issue Feb 15, 2021 · 7 comments
Open

Customization of evaluation of new child objects #331

estevao-schultz-neto-SAP opened this issue Feb 15, 2021 · 7 comments
Assignees
Labels
feature New feature or refactoring

Comments

@estevao-schultz-neto-SAP
Copy link
Contributor

estevao-schultz-neto-SAP commented Feb 15, 2021

Is it possible to exclude new objects created in an older container object to be checked? E.g.: Brand new method in an old class. Or even: brand new .INCLUDE in an old report. (reported by Rolf Mantel)

Answer: In the past, the tool was behaving like this. Then we received the requirement of inspecting only these new “objects” (instead of the whole wrapping/main object). Personally, we do not see any impediment on enforcing code quality and standards in this new piece of code. Many checks are anyway searching for real errors (e.g.: Empty Catches, Empty IFs, non-class based exceptions, modularization, nesting depth, usage of magical numbers, etc…). But we could evaluate a way of making this a customizable check parameter? E.g.: Check/Resolve Child Objects? I added this to our backlog under the label “feature” and discuss with my colleagues:

@estevao-schultz-neto-SAP estevao-schultz-neto-SAP added the feature New feature or refactoring label Feb 15, 2021
@lucasborin
Copy link
Member

IMO, if you are creating new objects within a legacy program / function module, it means you are implementing a new feature, and it should follow the clean code to avoid this feature being a 'legacy problem' in the future (not easy to test, not easy to maintain, not simple, etc).

Do you see any clear reason for it?

@RolfMantel
Copy link

There is a massive difference between "creating new objects" and re-factoring code":

  • When I create new objects, I can take care of clean code
  • for a minor re-factoring I cannot take care of clean code due to limited availability of development time

When I want to re-factor a horrible legacy object (e.g. one old method of 800 lines of code), the following steps are typical

  1. Break up one large object into several medium-size objects
  2. Completely re-factor some but not all of of these medium-size objects (these re-factored parts would typically be in one or 10 new classes)

If the "broken-up but not re-factored" objects need to comply to Code PAL, it makes partial re-factoring impossible, i.e. provides an unnecessary barrier that prevents re-factoring.

@lucasborin
Copy link
Member

There is a massive difference between "creating new objects" and re-factoring code":

  • When I create new objects, I can take care of clean code
  • for a minor re-factoring I cannot take care of clean code due to limited availability of development time

Hmm, I see both as codes that tomorrow will be the 'new' legacies. If you are refactoring a code, you probably target a better code than today with low maintenance in the future, feature isolation, easy to test, etc (Ps: clean code advantages).

When I want to re-factor a horrible legacy object (e.g. one old method of 800 lines of code), the following steps are typical

  1. Break up one large object into several medium-size objects
  2. Completely re-factor some but not all of of these medium-size objects (these re-factored parts would typically be in one or 10 new classes)

I would recommend you to, instead of breaking up all the objects, do small refactorings applying the new best practices. For instance, you can start moving the first block of logic to a new class using the object-oriented programming paradigm and the clean code philosophy. Then, on the next sprint, you can get the second block, etc.

In the past, I used to see this way of refactorings and it was even harder to implement OOP and Clean Code on it. Furthermore, you refactor the whole feature twice (or more). Maybe, it is one of my traumas, hehe.

If the "broken-up but not re-factored" objects need to comply to Code PAL, it makes partial re-factoring impossible, i.e. provides an unnecessary barrier that prevents re-factoring.

IMO, it is just a matter of refactoring plan. If you deliver the small interactions applying all the best practices, you won't face any problem during its release.

Feel free to share your thoughts.

@RolfMantel
Copy link

I do not live long enough to re-factor the large amount of code I am responsible for; I am aiming for retirement less than 20 years in the future.

I liken myself with a cathedral architect in the 1300's. I have inherited a half-finished cathedral and my prime aim is to have the cathedral 70% finished by the time I retire. My secondary aim is to build a beautiful cathedral.
The best-practice is the "gothic" style allowing me to build a really high church, flooded with lots of light. However, the half-finished cathedral was started in "Romanesque" style, with thick walls and small windows. My heart bleeds every day I walk through those dark halls but I cannot re-build everything, I can only let in a little bit more light by breaking out another small window here and there.

If I have to re-factor one 30-year old "FORM" of 1,500 or 2,000 lines, the first step is to make this FORM testable:
I need to be able to write unit tests that only cover a few hundred lines each, so I must break up this old FORM into 5 or 10 objects even before I know all the details.
As a second step I will be able to write unit tests with a meaningful code coverage for those 5 or 10 objects.
Even at this stage, I have provided significant benefit to code quality, and at this macro level, I have not had any time to care about clean code.
Only when a meaningful code coverage is established, I am able to do things with unexpected potential side effects like getting rid of global variables.

@RolfMantel
Copy link

It appears that there are two different usages of the tool with contradictory requirements:
As a "tool for refactoring" I might wish to validate new sub-objects.
As a"quality control gate" I need to exempt new sub-objects.

So I agree, a feature request to enable both variants is the correct solution for this.

@lucasborin lucasborin self-assigned this Dec 10, 2021
@lucasborin
Copy link
Member

It appears that there are two different usages of the tool with contradictory requirements: As a "tool for refactoring" I might wish to validate new sub-objects. As a"quality control gate" I need to exempt new sub-objects.

So I agree, a feature request to enable both variants is the correct solution for this.

@RolfMantel: Yep, I agree with you. It is a nice feature to be implemented, which allows the user to decide if the child objects are/aren't relevant for the scan. By the way, its first version was implemented in the next release (2.00.0), which is under development yet. Please, let me know if you want to pilot it so that I share what is required to do so. Ref #539.

@lucasborin lucasborin linked a pull request Dec 13, 2021 that will close this issue
@bjoern-jueliger-sap
Copy link
Member

Related to #556

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or refactoring
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants