HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppModerate

Most elegant way to write this loop

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thisloopwaywriteelegantmost

Problem

I have some code which looks like this:

do {
    hr = pEnumMgmt->Next(1, &mgmtObjProp, &ulFetched);
    if (hr == S_FALSE) { break; }
    llCbStorage += mgmtObjProp[0].Obj.DiffArea.m_llAllocatedDiffSpace;
} while (hr != S_FALSE);


It is bad because hr == S_FALSE is checked twice on every iteration.

I could write it like this:

while (true) {
    hr = pEnumMgmt->Next(1, &mgmtObjProp, &ulFetched);
    if (hr == S_FALSE) { break; }
    llCbStorage += mgmtObjProp[0].Obj.DiffArea.m_llAllocatedDiffSpace;
};


It is bad because it gives the impression that there exists no loop invariant (i.e. it could be an infinite loop, which would be a programmer error).

Finally, I could write it with gotos, but that is bad because gotos are considered harmful. Also, it gets really hard to check the correctness of the loop.

How would you write this loop and why?

Solution

My initial thought is to simply lose the break. While they have their place in switch-case statements, it is generally best to avoid them in loops unless they're used at the top of a loop and truly help improve readability. Even then, I tend to look for alternatives.

do {
    hr = pEnumMgmt->Next(1, &mgmtObjProp, &ulFetched);
    if (hr != S_FALSE) { 
        llCbStorage += mgmtObjProp[0].Obj.DiffArea.m_llAllocatedDiffSpace;
    }
} while (hr != S_FALSE);


Alternatively, you could just initialize hr prior to the while loop and avoid the extra check against S_FALSE every loop.

hr = pEnumMgmt->Next(1, &mgmtObjProp, &ulFetched);
while (hr != S_FALSE) {
    llCbStorage += mgmtObjProp[0].Obj.DiffArea.m_llAllocatedDiffSpace;
    hr = pEnumMgmt->Next(1, &mgmtObjProp, &ulFetched);
}


I don't like the following style of while loop as it utilizes a side-effect.

while ((hr = pEnumMgmt->Next(1, &mgmtObjProp, &ulFetched)) != S_FALSE) {
    llCbStorage += mgmtObjProp[0].Obj.DiffArea.m_llAllocatedDiffSpace;
}


Lastly, it may be worthwhile to consider a for loop here. I like the look of this because it cleanly separates the looping from the updates to llCbStorage.

for (hr = pEnumMgmt->Next(1, &mgmtObjProp, &ulFetched);
     hr != S_FALSE;
     hr = pEnumMgmt->Next(1, &mgmtObjProp, &ulFetched)) 
{
    llCbStorage += mgmtObjProp[0].Obj.DiffArea.m_llAllocatedDiffSpace;
}

Code Snippets

do {
    hr = pEnumMgmt->Next(1, &mgmtObjProp, &ulFetched);
    if (hr != S_FALSE) { 
        llCbStorage += mgmtObjProp[0].Obj.DiffArea.m_llAllocatedDiffSpace;
    }
} while (hr != S_FALSE);
hr = pEnumMgmt->Next(1, &mgmtObjProp, &ulFetched);
while (hr != S_FALSE) {
    llCbStorage += mgmtObjProp[0].Obj.DiffArea.m_llAllocatedDiffSpace;
    hr = pEnumMgmt->Next(1, &mgmtObjProp, &ulFetched);
}
while ((hr = pEnumMgmt->Next(1, &mgmtObjProp, &ulFetched)) != S_FALSE) {
    llCbStorage += mgmtObjProp[0].Obj.DiffArea.m_llAllocatedDiffSpace;
}
for (hr = pEnumMgmt->Next(1, &mgmtObjProp, &ulFetched);
     hr != S_FALSE;
     hr = pEnumMgmt->Next(1, &mgmtObjProp, &ulFetched)) 
{
    llCbStorage += mgmtObjProp[0].Obj.DiffArea.m_llAllocatedDiffSpace;
}

Context

StackExchange Code Review Q#3639, answer score: 10

Revisions (0)

No revisions yet.