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

[Consider decomposing complex conditions] bad use case example #319

Open
Jelena-P opened this issue Apr 21, 2023 · 1 comment
Open

[Consider decomposing complex conditions] bad use case example #319

Jelena-P opened this issue Apr 21, 2023 · 1 comment
Labels
Adjustment Of Rule The issue or PR proposes an adjustment of a rule or set of rules clean-abap

Comments

@Jelena-P
Copy link

Relevant sections of the style guide
https://github.com/SAP/styleguides/blob/main/clean-abap/CleanABAP.md#consider-decomposing-complex-conditions

Description
While I agree with "decomposing complex conditions" idea in general, it seems the example provided in this rule is not a good one.

Examples
In the example provided, I would not write code like in the "anti-pattern" to begin with. If my code is expecting some values, then I'd check that separately (using CHECK, for example) or delegate that check to the applies( ) methods.

To be honest, introducing intermediate variable here doesn't look like a good solution. Also XSDBOOL is not intuitive and doesn't improve code readability here either. Simply splitting complex IF condition into individual IF conditions or delegating checks to the methods seems like a better approach.

@Jelena-P Jelena-P added Adjustment Of Rule The issue or PR proposes an adjustment of a rule or set of rules clean-abap labels Apr 21, 2023
@bjoern-jueliger-sap
Copy link
Member

Agreed, the example is strange in multiple ways - while code snippets of course can't make as much sense as full code examples, the existence of both applies and fits methods alone is weird since I see no semantic difference between an example "applying" or "fitting".

The example for the rule about extracting conditions to their own methods directly after that also is weird because the names seem backward - that a method called is_provided should check whether something is_working as a sub-condition runs counter to the idea that method names should be meaningful. "Provided" means that something is provided, not that it is provided and works.

The rules as such are fine but the examples could be a lot better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Adjustment Of Rule The issue or PR proposes an adjustment of a rule or set of rules clean-abap
Projects
None yet
Development

No branches or pull requests

2 participants