NSWI170 Computer Systems

Guidelines to write a better C/C++ code

previous curse | all curses | next curse

Unforgivable curse #3: Do not Repeat Yourselves (keep it DRY)

Many teachers have repeated to you that you should not repeat yourselves (at least not in the code). This may be a generally good practice not to copy the same thing over and over at least when it comes to matters of the digital world.

Motivation

When you repeat (possibly copy-paste) the same block of code, it makes it significantly harder to change later as you need to make the same modification over and over. It is also quite error-prone since it takes only a small omission and the duplicated code will get out of sync. Furthermore, you often need to make small adjustments in the copied code. That makes it very difficult to read and maintain as the eyes clearly see the same pattern but have a hard time spotting the tiny differences.

Additionally, including dead code (unrelated to task and unused in the actual computation, even in comments) is seen as unnecessarily verbosity by saying things that are not required at all (instead of just repeating them). Dead code causes similar issues as repeated code: It creates visual clutter that drags the attention of the programmer away from the actual live code, and generally makes fast inspection and review of the program harder. In maintenance tasks, the dead code may trigger compilation errors that may render the program unusable despite the fact that all live code would work. Moreover, the dead code is typically untested and triggering it accidentally may result in serious errors.

Explanation

When you have a block of code that needs to be executed multiple times, there are two things you should consider right away:

• Use a (for)loop to execute the block multiple times. Sometimes it might be difficult to spot, how to deal with minor differences in each iteration, but many situations can be solved by (constant) lookup tables (arrays).
• Use functions to wrap repeatedly used blocks. This goes hand in hand with functional decomposition. If you learned that lesson well, this will come as second nature to you.

On the other hand, there is an important red flag that could indicate that you are not doing the reduction of repeated code correctly: If you need to place conditional branches (if-else statements) into the body of the loop or the function to distinguish individual loops/invocations, then something is terribly wrong and you should consider using a different approach.

Note that with the C/C++ skills you have gained in this course, it is not possible to eliminate all redundancies. For that, you would require OOP polymorphism, indirect function invocation (function pointers), or even templates. However, try to minimize the redundancies as much as possible (within a reason).

When your program contains code that is never going to be executed or used (not even in corner cases); delete it. Similarly, delete all comments that are not relevant to the current code.

Examples

The following application handles three buttons that operate three individual LEDs. When a button is down, the corresponding LED is turned on, and vice versa. The application is wired so that Button #1 operates LED #3, Button #2 LED #2, and Button #3 LED #1. As the good code part demonstrates, the duplicities can be easily removed by loops.

void setup() {
    initialize_pin(LED1_PIN, OUTPUT);
    initialize_pin(LED2_PIN, OUTPUT);
    initialize_pin(LED3_PIN, OUTPUT);
    initialize_pin(BUTTON1_PIN, INPUT);
    initialize_pin(BUTTON2_PIN, INPUT);
    initialize_pin(BUTTON3_PIN, INPUT);
}

void loop() {
    if (get_pin_state(BUTTON1_PIN) == DOWN) {
        set_pin_state(LED3_PIN, ON);
    } else {
        set_pin_state(LED3_PIN, OFF);
    }    
    if (get_pin_state(BUTTON2_PIN) == DOWN) {
        set_pin_state(LED2_PIN, ON);
    } else {
        set_pin_state(LED2_PIN, OFF);
    }
    if (get_pin_state(BUTTON3_PIN) == DOWN) {
        set_pin_state(LED1_PIN, ON);
    } else {
        set_pin_state(LED1_PIN, OFF);
    }
}

// UC2: Using constants; for consistency all should be in CAPITALS
constexpr int LEDS[] = { LED1_PIN, LED2_PIN, LED3_PIN };
constexpr int LEDS_SIZE = sizeof(LEDS) / sizeof(LEDS[0]);
constexpr int BUTTONS[]
    = { BUTTON3_PIN, BUTTON2_PIN, BUTTON1_PIN };
constexpr int BUTTONS_SIZE
    = sizeof(BUTTONS) / sizeof(BUTTONS[0]);

// UC1: Decomposition into functions: those things do logically different things, 
so they should be in different functions.

void initialize_pins() {
    for (int i = 0; i < LEDS_SIZE; ++i) {
        initialize_pin(LEDS[i], OUTPUT);
    }
}

void initialize_buttons() {
    for (int i = 0; i < BUTTONS_SIZE; ++i) {
        initialize_pin(BUTTONS[i], INPUT);
    }
}

void setup() {
    initialize_pins();
    initialize_buttons();
}

void loop() {
    for (int i = 0; i < BUTTONS_SIZE; ++i) {
        bool isDown = get_pin_state(BUTTONS[i]) == DOWN;
        set_pin_state(LEDS[i], isDown ? ON : OFF);
    }
}

In the second example, each of the three buttons is doing something else. Therefore, using a loop that will hold a branch for each button is quite bad. A better solution is to place the common parts into a function and then simply call that function individually for each button. The main() function of the good code example could be considered also slightly redundant (the handle_button() is called three times in the same manner); however, without function pointers or polymorphism, that is the best we can do here.

int main() {
    // ...
    for (int i = 0; i < BUTTONS_SIZE; ++i) {
        if (get_pin_state(BUTTONS[i]) == DOWN) {
            if (!button_was_down[i]) {
                button_was_down[i] = true;
                if (i == 0) {
                    start_the_countdown();
                } else if (i == 1) {
                    stop_the_countdown();
                } else {
                    reset_counter();
                }
            }
        } else {
            button_was_down[i] = false;
        }
    }
}

bool handle_button(int buttonIndex) {
    if (get_pin_state(BUTTONS[buttonIndex]) == DOWN) {
        if (!button_was_down[buttonIndex]) {
            button_was_down[buttonIndex] = true;
            return true;
        }
    } else {
        button_was_down[buttonIndex] = false;
    }
    return false;
}

int main() {
    // ...
    if (handle_button(0)) start_the_countdown();
    if (handle_button(1)) stop_the_countdown();
    if (handle_button(2)) reset_counter();
}

Finally, do not keep old or inactive code around, not even in comments. If you want to keep old versions, use a proper code versioning system locally (such as Git). The example below shows several common kinds of dead code that should be removed.

bool handle_button(int buttonIndex) {
    if (get_pin_state(BUTTONS[buttonIndex]) == DOWN) {
        if (!button_was_down[buttonIndex]) {
            //button_was_up[buttonNum] = false;
            button_was_down[buttonIndex] = true;
            return true;
        }
    } else {
        //button_was_up[buttonNum] = true;
        button_was_down[buttonIndex] = false;
    }
    return false;
}

//constexpr int LEDS_SIZE = 3;

int main_old() {
    for (int i = 0; i < LEDS_SIZE; ++i) {
        initialize_pin(LEDS[i], OUTPUT);
    }
    //run_blinking();
}

int main() {
    if (handle_button(0)) start_the_countdown();
    //if (handle_button(0)) reset_counter();
    if (handle_button(1)) stop_the_countdown();
    if (handle_button(2)) reset_counter();
}

bool handle_button(int buttonIndex) {
    if (get_pin_state(BUTTONS[buttonIndex]) == DOWN) {
        if (!button_was_down[buttonIndex]) {
            button_was_down[buttonIndex] = true;
            return true;
        }
    } else {
        button_was_down[buttonIndex] = false;
    }
    return false;
}

int main() {
    if (handle_button(0)) start_the_countdown();
    if (handle_button(1)) stop_the_countdown();
    if (handle_button(2)) reset_counter();
}