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