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

[EC2][PHP][5R] Avoid Multiple IfElse Statements #160

Closed
wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 6, 2023

Issue #121

@MP-Aubay MP-Aubay added 🗃️ rule rule improvment or rule development or bug php 🏆 challenge2023 🏆 Work done during the ecoCode Challenge 2023 labels Apr 7, 2023
@dedece35 dedece35 linked an issue Apr 9, 2023 that may be closed by this pull request
@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

93.9% 93.9% Coverage
0.0% 0.0% Duplication

Copy link
Member

@dedece35 dedece35 left a comment

Choose a reason for hiding this comment

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

please add also updates with this rule implementation :

another modification to do : add a tests in ecoCode-php-test-project ==> please check others tests and make the same. as you can see test files are quite identical to unit test resources files

please make corrections on code smell problems (sonarcloud report)


private void checkElseIfStatement(Tree tree) {
String ifTree = tree.toString();
String findStr = "elseif";
Copy link
Member

Choose a reason for hiding this comment

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

here, you find only elseif use casees but you can have else and if separated use case, and they won't be found.

}
BlockTree node = (BlockTree) parentNode;
int sizeBody = node.statements().toArray().length;
for(int i=0; i<sizeBody;++i){
Copy link
Member

Choose a reason for hiding this comment

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

the algorithm can be optimized :

  • you can use streams
  • you can use a loop and break it once count > 1, instead of loop on all elements for nothing
  • you can use foreach loop instead for loop, it should be more accurate


<h2>Non-compliant Code Example</h2>
<pre>
$index = 1;
Copy link
Member

Choose a reason for hiding this comment

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

please correct indentation

}
}

public function methodWithOneIfElseIf() {
Copy link
Member

Choose a reason for hiding this comment

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

please add another method test with separated else and if instead of elseif

@dedece35 dedece35 added the 👀 👀 review done 👀 👀 review done - waiting for changes label Apr 9, 2023
@dedece35 dedece35 assigned ghost Apr 9, 2023
@dedece35
Copy link
Member

dedece35 commented May 7, 2023

Hi @eliottlv,
please, could you :

  • resolve conflicts
  • take into account review notes

... in ordre to merge this PR.

@dedece35
Copy link
Member

dedece35 commented Jun 6, 2023

new PR #200 created to make the review modifications ourselves (because of inactivity)

PR closed due to recreation a local branch to edit code (because of inactivity in this PR)

@dedece35 dedece35 closed this Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗃️ rule rule improvment or rule development or bug 🏆 challenge2023 🏆 Work done during the ecoCode Challenge 2023 php 👀 👀 review done 👀 👀 review done - waiting for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants