Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have some code:

  $matchingEvents = [];
  foreach( $this->recurringEvents as $event ){
    if( $event['RCE_Type'] === 'Month' && $event['RCE_Day'] === $dayOfMonth ){
      array_push( $matchingEvents, $event );
    } else if( $event['RCE_Type'] === 'Week' && $event['RCE_Day'] === $dayOfWeek ){
      array_push( $matchingEvents, $event );
    }
  }

Which duplicates the line:

array_push( $matchingEvents, $event );

This is small duplication, but still duplication. I could reformat it as this:

  $matchingEvents = [];
  foreach( $this->recurringEvents as $event ){
    if( ( $event['RCE_Type'] === 'Month' && $event['RCE_Day'] === $dayOfMonth )
        ||
        ( $event['RCE_Type'] === 'Week' && $event['RCE_Day'] === $dayOfWeek ) ){
      array_push( $matchingEvents, $event );
    }
  }

But I feel like this drastically reduces the readability of the code.

In this type of situation, is DRY or simple more important?

share|improve this question
1  
You can improve the readability of the (||) version by judicious use of white space to express the tree of connectives. Use the mathematical structure to your advantage. Don't fight the math. It will always win. –  nomen Apr 26 at 19:35
    
@nomen: What "mathematical structure" are you referring to? Care to elaborate? –  Elias Van Ootegem Apr 30 at 7:12
    
@Elias: the tree of logical connectives. –  nomen Apr 30 at 16:48

4 Answers 4

up vote 3 down vote accepted

While I agree that is understandable enough with || symbols, I think it could be greatly improved with some well-named variables.

For example:

$matchingEvents = [];
foreach( $this->recurringEvents as $event ){
    $matchesMonth = $event['RCE_Type'] === 'Month' && $event['RCE_Day'] === $dayOfMonth
    $matchesDayOfWeek = $event['RCE_Type'] === 'Week' && $event['RCE_Day'] === $dayOfWeek

    if( $matchesMonth || $matchesDayOfWeek ){
        array_push( $matchingEvents, $event );
    }
}
share|improve this answer

I think that this is a matter of preference.

However, as a programmer, when I see that || it automatically translates to an "OR" in my head, so it's essentially the same thing as seeing else if. In fact, there's more spacing between the lines, so in my humble oppinion, the latter using || is actually easier to read.

share|improve this answer

You could move the matching test to a separate method, like this:

private function is_matching_day( $event, $dayOfWeek, $dayOfMonth ) {

    if ( $event['RCE_Type'] === 'Month' && $event['RCE_Day'] === $dayOfMonth )
        return true;

    return $event['RCE_Type'] === 'Week' && $event['RCE_Day'] === $dayOfWeek;
}

Then your foreach statement would be more readable, and the duplicated array_push() is gone:

$matchingEvents = [];
foreach( $this->recurringEvents as $event ) {       
    if ( $this->is_matching_day( $event, $dayOfWeek, $dayOfMonth ) )
        array_push( $matchingEvents, $event );
}
share|improve this answer

But I feel like this drastically reduces the readability of the code.

I think it improves readability. If someone reads ... else if ..., they will assume that different things happen in the two cases. It will take some time for them to understand that the same thing happens in two different conditions.

The version with ... || ..., on the other hand, clearly suggests that there are two different conditions for the same thing to happen.

If you feel that the two-line condition is too long to be readable, toscho's suggestion to put it into a separate function is good. Maybe you even want to reuse the same function in other parts of the code?

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.