Prev: OID_802_11_BSSID_LIST_SCAN
Next: get unicode character OEMTextOut
From: BobF on 1 Jun 2005 08:43
On Wed, 01 Jun 2005 13:50:11 +0200, Robert Marquardt wrote:

> BobF wrote:
>
>> I've seen this - and seen it debated - a lot. One could argue that the
>> four successive if blocks make the code easier to follow. Everything you
>> need to understand is in front of you. 'return oops()' forces the reader
>> to some other place in the code, or even another file to figure out the
>> functionality.
>
> You are wrong.

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

> If function oops() or a macro OOPS() are equivalent in
> understanding the functionality.
> What i dislike is the 4 lines needed (plus the empty line). With 5
> blocks you get a staggering 25 lines where about 5 would be sufficient.

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.
From: Wan-Hi on 1 Jun 2005 11:55
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'.
From: Maxim S. Shatskih on 2 Jun 2005 02:32
> depth to which you must nest 'if' statements. Version 1.0 of most programs
> and drivers can and should be discarded for a better design later.

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

> Commercial pressures make most of us keep the old code, so then you just
> have to improve it block at a time as you have the time or you need to
> modify or fix a particular area.

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(a)storagecraft.com
http://www.storagecraft.com


From: Maxim S. Shatskih on 2 Jun 2005 02:36
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(a)storagecraft.com
http://www.storagecraft.com

"Wan-Hi" <WanHi(a)discussions.microsoft.com> wrote in message
news:1F0C555F-F0DA-44FB-A3FD-2F791F54F2CE(a)microsoft.com...
> the fact that excessive if block 'nesting' causes maintainance problems was
> the very reason why i reconsidered my way of failure checks and asked you.
>
> i don't really believe that it's generally caused by a bad algorithm.
> logically
>
> if (failure)
> {
> do clean up;
> show error;
> return;
> }
> else
> {
> if (nextFailure)
> {
> do clean up;
> show error;
> return;
> }
> else
> {
> ...
> }
> }
>
> is logically more correct than a code avoiding 'else' blocks. in my opinion,
> functioning code avoiding 'else' blocks is a result and benefit of the
> language specific 'code execution path'. so the algo is not that bad, but the
> design is. this does not mean that i hold up the opinion to use 'else' if
> unnecessary. i also second you that i should reconsider my design when i
> encounter excessive if block 'nesting'.
>
> "David Craig" wrote:
>
> > If you are coding if blocks within if blocks more than three or so levels
> > deep, you need to reconsider your algorithms and design. It is not
> > necessary. It leads to bad results since the code becomes far more
> > difficult to debug, understand, and maintain. Most bad code can be fixed
> > with better algorithms and a redesign. That 'do once' block helps limit
the
> > depth to which you must nest 'if' statements. Version 1.0 of most programs
> > and drivers can and should be discarded for a better design later.
> > Commercial pressures make most of us keep the old code, so then you just
> > have to improve it block at a time as you have the time or you need to
> > modify or fix a particular area.


From: Pavel A. on 4 Jun 2005 19:52
"Wan-Hi" wrote:
> 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'.

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



First  |  Prev  | 
Pages: 1 2 3 4 5
Prev: OID_802_11_BSSID_LIST_SCAN
Next: get unicode character OEMTextOut