4
votes
\$\begingroup\$

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?

\$\endgroup\$

locked by Jamal Jan 29 '15 at 18:36

This question exists because it has historical significance, but it is not considered a good, on-topic question for this site so please do not use it as evidence that you can ask similar questions here. This question and its answers are frozen and cannot be changed. See the help center for guidance on writing a good question.

Read more about locked posts here.

  • 2
    \$\begingroup\$ I would go with the while (true): all alternatives have some duplicated code somewhere. \$\endgroup\$ – Sjoerd Jul 27 '11 at 23:13
10
votes
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ I went for the for loop, it is brilliant -- looks very clear and does exactly what it should with minimal code replication. \$\endgroup\$ – Felix Dombek Jul 26 '11 at 15:55
  • 1
    \$\begingroup\$ I tend to like the while loop, while it uses a side-effect, it is idiomatic c/c++ \$\endgroup\$ – Keith Nicholas Jul 26 '11 at 22:01
  • \$\begingroup\$ My preference is the while loop with side-effecting condition. That's a pretty common C idiom, and it avoids duplication, and it cleanly separates condition from body. The one change I'd make is to have it say while (S_FALSE != (hr = pEnumMgmt->Next(1, &mgmtObjProp, &ulFetched))) \$\endgroup\$ – Phil Miller May 20 '12 at 14:58
10
votes
\$\begingroup\$

COM is like a zombie -- the only way to be safe with it around is to lock it in a box from which it can't escape, then bury it as deeply as possible, just to be sure.

Since (in this case) you're iterating over a COM pseudo-collection, to make this into decent C++, you want to lock the COM zombie-iteration inside a real iterator, something like this:

template <class T>
class COM_iterator { 
    HRESULT current;
    unsigned long fetched;
    XXX mgmtObject;    // not sure of the correct type here.
    T enumerator;

    COM_iterator next() {
        current = enumerator->Next(1, &mgmtObject, &fetched);
        return *this;
    }
public:
    COM_iterator() : current(S_FALSE) {}

    COM_iterator(T enum) : enumerator(enum) {
        next();
    }

    COM_iterator &operator++() { return next(); }

    unsigned long long operator*() {
        return mgmtObjProp[0].Obj.DiffArea.m_llAllocatedDiffSpace; 
    }

    bool operator!=(COM_iterator const &other) {
       return current != other.current;
    }
};

We then want to bury that ugliness in a header file. Using it, however, we can not merely produce a prettier loop, but in fact eliminate the loop itself entirely, and instead use an idiomatic C++ algorithm:

#include <numeric>
#include "com_iterator"

long long total_bytes = 
    std::accumulate(COM_iterator<your_COM_type>(pEnumMgmt),
                    COM_iterator<your_COM_type>(),
                    0LL);

As it stands, this code does have one real problem: it's only suitable for looking at one particular property of the data you're looking at. To clean that up, it should probably have the code to access the specific data involved (mgmtObjProp[0].Obj.DiffArea.m_llAllocatedDiffSpace, in this case) encapsulated in a separate policy-like class that's passed as a template parameter to the iterator. This allows separation between the COM iteration code, and the code to access the internals of the specific object you care about. Alternatively, the COM_iterator could return mgmtProp, and leave it to the client code to figure out what data to use from each item being iterated over.

Edit: Using the "policy-like" extractor class, you'd start by creating a small class to extract and return the data of interest:

class extract_AllocatedDiffSpace { 
    XXX mgmtObject;
public:
    extact_AllocatedDiffSpace(XXX init) : mgmtObject(init) {}
    operator long long() { return mgmtObject[0].Obj.DiffArea.m_llAllocatedDiffSpace; }
};

... and then you'd add a template parameter for that type:

template <class T, class U>
class COM_iterator { 
// ...
};

and rewrite operator * something like this:

U operator *() { 
    return U(mgmtProp);
}

Then the extractor object would be returned by operator *, and when std::accumulate tried to add that to long long, it would convert from the extract_whatever object to long long using your operator long long, which (in turn) would extract and return the right field.

You do need/want to treat this with a bit of care -- the extract_xxx class should generally have only a ctor and one cast operator, to prevent it from accidentally being used in unintended ways. If, for example, you already have a class that acts as a front-end for an mgmtObject in other ways, you probably do not want to just add the operator long long to it -- that makes it much too easy for that conversion to be used in other contexts where it's not appropriate.

\$\endgroup\$
  • \$\begingroup\$ I really like this solution although I have not implemented it yet. Could you give me a hint on how you would write the policy-like class? Thanks! \$\endgroup\$ – Felix Dombek Jul 27 '11 at 23:57
  • \$\begingroup\$ +1 Nice approach. How would you handle something like E_FAIL being returned from Next? Throw an exception? \$\endgroup\$ – Adrian McCarthy Jul 29 '11 at 20:31
  • \$\begingroup\$ @Adrian: Yes, probably. E_FAIL, E_ABORT, E_POINTER, and probably a few more, look/feel a lot like exceptions to me. \$\endgroup\$ – Jerry Coffin Jul 29 '11 at 20:52
  • \$\begingroup\$ That sounds really nice (honestly, I mean this), but it exhibits why some people think C++ is ridiculous and elephantine amazingly well. A 3 line loop is being replaced with 10s of lines of code, including 2 classes (one of them templated), and no loop. \$\endgroup\$ – Phil Miller May 20 '12 at 15:03
  • \$\begingroup\$ @Novelocrat: First, by my count, the original loop is 5 lines, not three. Second, that 10s of lines of other code is only written once. That lets you use all the normal C++ algorithms, so most "loops" end up as a line apiece. For anything but the most trivial program, the code ends up both shorter and much more readable for anybody accustomed to using algorithms/iterators in other code (looking only at length, the break-even point is about 7 loops). \$\endgroup\$ – Jerry Coffin May 21 '12 at 0:26
4
votes
\$\begingroup\$

You could combine your loop condition and the hr assignment in one line, like this:

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

the above is in my opinion the cleaner way to do what you want.. I don't like the following way because it's more long than necessary:

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.