Complex boolean logic vs DRY

Question

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?

• nomen: 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.
• Elias Van Ootegem: @nomen: What "mathematical structure" are you referring to? Care to elaborate?
• nomen: @Elias: the tree of logical connectives.

Answer 1

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

Answer 2

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

Answer 3

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.

Answer 4

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?