Readability vs. Maintainability: Condensing statements to loops

Question

Firstly, an example of what I'm referring to:

UINT f, i, s;
CONST UINT k[5] = { VK_LBUTTON, VK_RBUTTON, VK_MBUTTON, VK_XBUTTON1, VK_XBUTTON2 };

for (f = RI_MOUSE_LEFT_BUTTON_DOWN, i = 0, s = RI_KEY_MAKE; f != RI_MOUSE_WHEEL; f <<= 1, i += s, s = !s)
    if ((pMouseData->usButtonFlags & f) == f)
        SetVKeyState(k[i], s);

Compared to this:

if (pMouseData->usButtonFlags & RI_MOUSE_LEFT_BUTTON_DOWN)
    SetVKeyState(VK_LBUTTON, RI_KEY_MAKE);

if (pMouseData->usButtonFlags & RI_MOUSE_LEFT_BUTTON_UP)
    SetVKeyState(VK_LBUTTON, RI_KEY_BREAK);

if (pMouseData->usButtonFlags & RI_MOUSE_RIGHT_BUTTON_DOWN)
    SetVKeyState(VK_RBUTTON, RI_KEY_MAKE);

if (pMouseData->usButtonFlags & RI_MOUSE_RIGHT_BUTTON_UP)
    SetVKeyState(VK_RBUTTON, RI_KEY_BREAK);

if (pMouseData->usButtonFlags & RI_MOUSE_MIDDLE_BUTTON_DOWN)
    SetVKeyState(VK_MBUTTON, RI_KEY_MAKE);

if (pMouseData->usButtonFlags & RI_MOUSE_MIDDLE_BUTTON_UP)
    SetVKeyState(VK_MBUTTON, RI_KEY_BREAK);

if (pMouseData->usButtonFlags & RI_MOUSE_X1_BUTTON_DOWN)
    SetVKeyState(VK_XBUTTON1, RI_KEY_MAKE);

if (pMouseData->usButtonFlags & RI_MOUSE_X1_BUTTON_UP)
    SetVKeyState(VK_XBUTTON1, RI_KEY_BREAK);

if (pMouseData->usButtonFlags & RI_MOUSE_X2_BUTTON_DOWN)
    SetVKeyState(VK_XBUTTON2, RI_KEY_MAKE);

if (pMouseData->usButtonFlags & RI_MOUSE_X2_BUTTON_UP)
    SetVKeyState(VK_XBUTTON2, RI_KEY_BREAK);

The later code is obviously a completely unrolled loop.

This is really a question over whether readability or reducing redundancy (as in repeated statements in different areas of the code) is better in the long run.

I'm more concerned about visiting it later in the context of a large code-base. Surely reducing the number of areas to modify is a good thing; does it matter the initialisation and update statements for the for loop are a little messy?

Has anyone experience in this, probably trivial, issue.

Answer 1

The key is to condense only statements that are completely alike:

const UINT k[5] = { VK_LBUTTON, VK_RBUTTON, VK_MBUTTON, VK_XBUTTON1, VK_XBUTTON2 };

static_assert( (RI_MOUSE_LEFT_BUTTON_DOWN << 2) == RI_MOUSE_RIGHT_BUTTON_DOWN );

for ( UINT i = 0; i < _count_of(k); ++i ) {
    if (pMouseData->usButtonFlags & (RI_MOUSE_LEFT_BUTTON_DOWN << 2*i))
        SetVKeyState(k[i], RI_KEY_MAKE); 
    if (pMouseData->usButtonFlags & (RI_MOUSE_LEFT_BUTTON_UP << 2*i)))
        SetVKeyState(k[i], RI_KEY_BREAK); 
}

All the comma operators are gone, unusual loop increment is gone, symbolic variables are still used for the key state.

I think this is actually easier to read than the original, because it fits on one page of code, and the repetition is obvious.

EDIT: And now the flag-relation assumption is documented.

I might actually go as far as:

struct { UINT vk;      UINT downflag;                 UINT upflag;
} const k[] = {
       { VK_LBUTTON,   RI_MOUSE_LEFT_BUTTON_DOWN,     RI_MOUSE_LEFT_BUTTON_UP },
       { VK_RBUTTON,   RI_MOUSE_RIGHT_BUTTON_DOWN,    RI_MOUSE_RIGHT_BUTTON_UP },
    ...
};
for ( UINT i = 0; i < _count_of(k); ++i ) {
    if (pMouseData->usButtonFlags & k[i].downflag)
        SetVKeyState(k[i].vk, RI_KEY_MAKE); 
    if (pMouseData->usButtonFlags & k[i].upflag)
        SetVKeyState(k[i].vk, RI_KEY_BREAK); 
}

in order to remove the assumption about the flags using adjacent bits in the correct order.

You could use the latter version making the second argument to SetVKeyState one of the table columns, but IMO that loses the valuable pairing structure.