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???

Answer 1

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));
}

Answer 2

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
} 

Answer 3

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

Answer 4

$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);

Answer 5

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);
}

Answer 6

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);
}

Answer 7

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));