patterncppModerate
Most elegant way to write this loop
Viewed 0 times
thisloopwaywriteelegantmost
Problem
I have some code which looks like this:
It is bad because
I could write it like this:
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
How would you write this loop and why?
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.
Alternatively, you could just initialize hr prior to the while loop and avoid the extra check against S_FALSE every loop.
I don't like the following style of while loop as it utilizes a side-effect.
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.
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.