Code Smell Alert: A common code pattern that I see is where the if/else logic and the if conditional is backwards. I call this the “if/else backwards code pattern.” This code pattern makes the code less readable, which decreases its quality. There are two clues to let you know that you have stumbled into this pattern:
- The
ifconditional is set off of afalsestate. Typically you’ll see anotoperator (!) as your clue. - The
iflogic is setting a default state. But that is the job of theelse.
Smelly Version
This is the smelling code pattern:
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| <?php | |
| namespace KnowTheCode; | |
| /** | |
| * Smelly code version. Why? | |
| * | |
| * 1. The `if` conditional expression is working on a falsey state. | |
| * 2. The work is being done in the `else` code block. | |
| * | |
| * @since 1.0.0 | |
| * | |
| * @param string $event_name | |
| * | |
| * @return int | |
| */ | |
| function count_number_of_times_fired( $event_name ) { | |
| static $events = array(); | |
| if ( ! isset( $events[ $event_name ] ) ) { | |
| $events[ $event_name ] = 1; | |
| } else { | |
| $events[ $event_name ]++; | |
| } | |
| return $events[ $event_name ]; | |
| } | |
| d( count_number_of_times_fired( 'loop_start' ) ); |
Refactored Version
This is the refactored code, showing you the right order to setup the if/else. Notice that the if conditional expression is now testing for a “truthy” state and the code within the if’s code block is doing work. Also notice that the else is now doing the default work.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| <?php | |
| namespace KnowTheCode; | |
| /** | |
| * This version shows the right way to do the if/else logic. Notice | |
| * that the if conditional expression is testing for a "truthy" condition | |
| * and the work is being done in it's code block. | |
| * | |
| * The else is now set to default state. | |
| * | |
| * @since 1.0.0 | |
| * | |
| * @param string $event_name | |
| * | |
| * @return int | |
| */ | |
| function count_number_of_times_fired( $event_name ) { | |
| static $events = array(); | |
| if ( isset( $events[ $event_name ] ) ) { | |
| $events[ $event_name ]++; | |
| } else { | |
| $events[ $event_name ] = 1; | |
| } | |
| return $events[ $event_name ]; | |
| } | |
| d( count_number_of_times_fired( 'loop_start' ) ); |
Did you learn something new? Share your thoughts.