2
\$\begingroup\$

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?

\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – nomen Apr 26 '14 at 19:35
  • \$\begingroup\$ @nomen: What "mathematical structure" are you referring to? Care to elaborate? \$\endgroup\$ – Elias Van Ootegem Apr 30 '14 at 7:12
  • \$\begingroup\$ @Elias: the tree of logical connectives. \$\endgroup\$ – nomen Apr 30 '14 at 16:48
4
\$\begingroup\$

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 );
    }
}
\$\endgroup\$
5
\$\begingroup\$

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 );
}
\$\endgroup\$
5
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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