CODEWITHSUNDEEP
phpcoding-style
Zak Henry
Zak Henry

Reputation: 2185

Coding style - how do I format a long if condition to make it readable

I have a long if condition as follows. There are two conditions that both have to not be met, for the statement to evaluate. I did have it as a one liner with a lot of && and ! but it became unreadable. I have tried splitting it into an if elsif else, which is more readable but doesn't read well, as the first if elsif blocks have no code in them.

What would be the best practice to tidy this code block?

if ($instructionObject->instruction=='nesting_grammar' && $instructionObject->match=='>'){ //if instruction is a '>' child indicator
   //don't change the child depth
}else if ($instructionObject->instruction=='selector' && is_object($this->instructions[$key+1]) && $this->instructions[$key+1]->instruction == 'nesting_grammar' && $this->instructions[$key+1]->match == '>'){ //if instruction is a selector followed by a '>'
   //don't change the child depth
}else{
   $insertOffset += $childDepth;
   unset($childDepth);
}

Upvotes: 2

Views: 1608

Answers (5)

hudolejev
hudolejev

Reputation: 6018

Not directly answering your question, but what about something like:

if (my_check($instructionObject) || $instructionObject->instruction=='selector' && my_check($this->instructions[$key+1])) {
} else {
   $insertOffset += $childDepth;
   unset($childDepth);
}

function my_check($obj) {
    return is_object($obj) && $obj->instruction == 'nesting_grammar' && $obj->match == '>';
}

-- you are basically doing the same thing twice, time to think about a function for that.

Upvotes: 1

sectus
sectus

Reputation: 15464

You can use "extract method" refactoring. Replace your conditions to new methods.

if ($this->isInstructionNestingGrammar($instructionObject)){ 
   //don't change the child depth
}else if ($this->isIntructionSelect($instructionObject)){ 
   //don't change the child depth
}else{
   $insertOffset += $childDepth;
   unset($childDepth);
}

In new methods put every compare to separate line.

P.S. Don't be afraid of long name of methods.

Upvotes: 3

Blorgbeard
Blorgbeard

Reputation: 103525

Pull out sub-expressions into variables. Pseudo-example:

flibjit = FlibjitManager.FlibjitInstance(this);
isFrob = 
    (flibjit.Froblocity >= FlibjitManager.FrobThreshold) &&   
    (flibjit.Type == FlibjitTypes.Frobby);

if (isFrob) {
   // ...

Upvotes: 0

Chris Brown
Chris Brown

Reputation: 4635

Personally if i'm going to span the check across multiple lines i lay it out similar to how i'd lay out a JavaScript object;

if (
    great big long check line goes in here &&
    another really long ugly check line goes in here too
) {
   // Do this code
}
else if (
    check 3 &&
    check 4
) {
    //Do this code
}

Upvotes: 0

jtheman
jtheman

Reputation: 7491

Just negate the conditions and skip the if and else if parts as the two initial conditions don't do anything...

if (
     !($instructionObject->instruction=='nesting_grammar' && 
       $instructionObject->match=='>') 
    || !($instructionObject->instruction=='selector' 
        && is_object($this->instructions[$key+1]) 
        && $this->instructions[$key+1]->instruction == 'nesting_grammar' 
        && $this->instructions[$key+1]->match == '>')
 ) {
   $insertOffset += $childDepth;
   unset($childDepth);
 }

Upvotes: 1

Related Questions