On Wed, 01 Jun 2005 13:50:11 +0200, Robert Marquardt wrote:

Wow ... lots of room for differing perspectives here, I see :-)

I'll agree in the case where the author is the one reading the code. When
its some poor person trying to acquaint themselves at some point later, I
don't necessarily agree.

Now, if oops() (or OOPS()) are a widely used convention throughout a large
project, then I agree completely.

i thank everyone sharing his / her thoughts. it's clear to me now that proper
device io code should expect all failures, even the 'exotic' ones. the
discussion about everyone's favourite failure checking loop was quite
informative. not knowing some of the expressions i read, e.g. k&r (?), i
decided to go with 'while', empowering the user to decide when to quit and
when to retry:

// Retrieving the device info set of currently installed devices exposing
// the volume interface.
while(true)
{
hVolInfoSet = SetupDiGetClassDevs(&GUID_DEVINTERFACE_VOLUME,
NULL,
NULL,
DIGCF_DEVICEINTERFACE |
DIGCF_PRESENT);

if (hVolInfoSet != INVALID_HANDLE_VALUE)
{
break;
}
else
{
switch(ShowErrorMessage(GetLastError(), MB_RETRYCANCEL))
{
case IDRETRY:
continue;
default:
return false;
}
}
}

although my first problem of nested blocks didn't vanish, i find this way of
failure checking easier to read. so i think i'll favourize code robustness
and easier readibility over code line 'packing'.

Not affordable in general due to PITAs to debug the new code - the old one was
stable. Large timeframes.

That's why "refactoring" was invented. Minor remakes, each standartized a lot,
which change no functionality and no performance, but just make the code
clearer. This is especially necessary for OO languages (bad OO code is much
better then bad C code).

The usual refactoring steps are like - splitting the function to 2, moving
functions from class to class ("envious" function - defined in class B and uses
lots of internal details of class A - move it to class A), and so on.

Refactoring can be done step-by-step without rewriting from scratch. Each minor
step will hardly introduce a bug, but, as a cumulative effect of lots of such
steps the code starts to be much better.

Surely all of this is not an issue if you need extreme performance. In these
cases, manual loop unrolling is good, using if() goto... instead of if() {...}
is usually good, "the most popular execution path must be jump-free", and so
on.

Coding for extreme performance is a talent. BTW - Linux code is good in this
aspect, though it is poor in being structured - lots of gotos etc.

--
Maxim Shatskih, Windows DDK MVP
StorageCraft Corporation
maxim@storagecraft.com
http://www.storagecraft.com

With the high-performance requirements, the usual way is

if( !condition ) goto Error;

with the Error: label being long below in the very end of the function. In
this case, you will avoid 1 jump for successful condition (which is nearly 100%
so - the condition is just a guard of malformed input data and exists for being
defensive only).

--
Maxim Shatskih, Windows DDK MVP
StorageCraft Corporation
maxim@storagecraft.com
http://www.storagecraft.com

"Wan-Hi" <WanHi@discussions.microsoft.com> wrote in message
news:1F0C555F-F0DA-44FB-A3FD-2F791F54F2CE@microsoft.com...

"Wan-Hi" wrote:
Pardon me but now this code fragment looks
ever more sloppy and less readable.

A sequence of actions that can (unlikely) fail, IMHO should look
exactly like what it is: a sequence - wrapped in try-except or
try-finally, or a function that returns success or failure.

Each element of the sequence can be wrappend into a checker macro
like in my previous post.

If you want to display a messagebox, fine.
Then the checker macro becomes a bit more complex;

#define CHECK( expr ) \
while(1) { \
if( check_helper(expr) ) break; /*success*/ \
if( ID_CANCEL != Msgbox( #expr ) ) { /*user wants continue*/ \
continue; \
} \
return false; /* or throw */ \
}

Now the sequence becomes:

try {

CHECK( hVolInfoSet = SetupDiGetClassDevs(...) );
CHECK( b = SetupDiSomething(...) );
CHECK( b = SetupDi...(...) );
CHECK( b = SetupDi...(...) );
// etc
} catch (...) ...

Of course tastes vary. Maybe there still are developers
payed for lines of code or number of operators...

--PA