how should remove else blocks

Question

PHPMD is telling me that I should avoid else block in this test, but in those case, I can't find a way to remove them.

Here is the code:

if ($fight->c1 == NULL) {
    if ($fight->c2 == NULL) {
        // C1 and C2 Is Bye
        $this->assertEquals($parentFight->$toUpdate, NULL);
    }
    else {
        // C1 Is Bye
        $this->assertEquals($parentFight->$toUpdate, $fight->c2);
    }
}
else {
    if ($fight->c2 == NULL) {
        // C2 Is Bye
        $this->assertEquals($parentFight->$toUpdate, $fight->c1);
    }
    else {
        // C1 and C2 Are all set
        $this->assertEquals($parentFight->$toUpdate, NULL);
    }
}

Any Idea???

is // C1 and C2 Are all set :: $this->assertEquals($parentFight->$toUpdate, null); OR $this->assertEquals($parentFight->$toUpdate, $flight->c2); ? — Shan, May 22 '17 at 06:47

score 1 · Answer 1 · answered May 22 '17 at 06:35

There is another way to do this :

if(($fight->c1 == null && $fight->c2 == null) || ($fight->c1 != null && $fight->c2 != null)) {
    // C1 and C2 Is Bye
    // C1 and C2 Are all set
    $this->assertEquals($parentFight->$toUpdate, null);
} else if($fight->c1 == null && $fight->c2 != null) {
    // C1 Is Bye
    $this->assertEquals($parentFight->$toUpdate, $fight->c2);
} else if($fight->c1 != null && $fight->c2 == null) {
    // C2 Is Bye
    $this->assertEquals($parentFight->$toUpdate, $fight->c1);
}

score 1 · Accepted Answer · answered May 22 '17 at 06:49

It could also be done with a ternary operator, something like this.

if (!$fight->c1) {
    $this->assertEquals($parentFight->$toUpdate, ($fight->c2 ?: null));
}

if (!$fight->c2) {
    $this->assertEquals($parentFight->$toUpdate, ($fight->c2 ?: null));
}

score 1 · Answer 3 · answered May 22 '17 at 06:55

1

$checkValue = null;
$cntNulls = (int)is_null($fight->c1) + (int)is_null($fight->c2);
if ($cntNulls === 1) {
    $checkValue = is_null($fight->c1) ? $fight->c2 : $fight->c1;
}

$this->assertEquals($parentFight->$toUpdate, $checkValue);

answered May 22 '17 at 06:55

maximkou

5,252
1
20
41

score 1 · Answer 4 · answered May 23 '17 at 07:15

It seems like when $fight->c1 not null, you want to pass $fight->c1. And when $fight->c2 not null, you want to pass $fight->c2. And when both are null you want to pass null.

What you'd simply do is,

$param = null;
if($fight->c1 != null)
{
    $param = $fight->c1;
}
if($fight->c2 != null)
{
    $param = $fight->c2;
}

$this->assertEquals($parentFight->$toUpdate, $param);

But I'd go one step further and abstract the $param resolving process like,

private function relolveParam($fight) {
    $param = null;
    if($fight->c1 != null)
    {
        $param = $fight->c1;
    }
    if($fight->c2 != null)
    {
        $param = $fight->c2;
    }
    return $param;
}

Then you're only end up with,

$this->assertEquals($parentFight->$toUpdate, $this->relolveParam($fight));

score 0 · Answer 5 · answered May 22 '17 at 06:24

Use else if rather then multiple if...else

if ($fight->c1 == null && $fight->c2 == null) {
    // C1 and C2 Is Bye
    $this->assertEquals($parentFight->$toUpdate, null);
} else if($fight->c1 == null &&  $fight->c2 != null) {
    // C1 Is Bye
    $this->assertEquals($parentFight->$toUpdate, $fight->c2);
} else if($fight->c1 != null &&  $fight->c2 == null) {
    // C2 Is Bye
    $this->assertEquals($parentFight->$toUpdate, $fight->c1);
} else {
    // C1 and C2 Are all set
    $this->assertEquals($parentFight->$toUpdate, null);
}

score 0 · Answer 6 · answered May 22 '17 at 06:38

0

You can use two if{} to instead of if{}else{} like this,

if(a){
  //do a
}else{
  //do !a
}

if(a){
  //do a
}
if(!a){
  //do !a
}

answered May 22 '17 at 06:38

LF00

27,015
29
156
295

score 0 · Answer 7 · answered May 22 '17 at 06:51

0

You could also make one test for each of the cases you are testing for, having 4 clear tests instead of one test where it is not obvious how all paths are tested

answered May 22 '17 at 06:51

rypskar

2,012
13
13

how should remove else blocks

7 Answers7