A big if
-
Thomas Weller wrote:
This is a quite good definition of what Dispose pattern is in C#...
It even better describes the RAII idiom in C++ :)
Thomas Weller wrote:
It is hard to follow if it gets lengthy
It does, but at least it is cleanly separated: the "normal path" is in the
ifpart, and the error handling in theelsepart. With the "pipe" model, both code paths interrupt each other and thats really messy and error prone.Thomas Weller wrote:
Error probability increases dramatically with every level of nesting - especially when it comes to maintenance.
How come? There is no copy-paste code and if something needs to be changed, it needs to be changed in one place. With the "pipe" model, if you add a new resource allocation, you need to make sure that it is released in each return path.
Thomas Weller wrote:
This sort of coding simply does not well with monitor space. Lines are indented for every nesting level - and soon you have to scroll horizontally only for reading source code!
No argument here, except that most editors have this secret little feature called "line wrapping" :)
Thomas Weller wrote:
Readability and maintainability issues.
Exactly the same arguments I have for the opposite argument - don't you love programming discussions? :laugh:
if ()
{
if ()
{
if ()
{
if ()
{
if ()
{
I Fail
to see
why
this
is any
less a
horror
becaus
e it
was
line
wrappe
d
automa
ticall
y.
}
else
{} } else { } } else { }}
else
{}
}
else
{}
Today's lesson is brought to you by the word "niggardly". Remember kids, don't attribute to racism what can be explained by Scandinavian language roots. -- Robert Royall
-
if ()
{
if ()
{
if ()
{
if ()
{
if ()
{
I Fail
to see
why
this
is any
less a
horror
becaus
e it
was
line
wrappe
d
automa
ticall
y.
}
else
{} } else { } } else { }}
else
{}
}
else
{}
Today's lesson is brought to you by the word "niggardly". Remember kids, don't attribute to racism what can be explained by Scandinavian language roots. -- Robert Royall
Exactly my point, unless I did not have the idea of putting it that way. :-D Regards Thomas
-
Nemanja Trifunovic wrote:
don't you love programming discussions?
:laugh: Sure, always ... But tell me two things: 1. What the heck is the RAII idiom in C++ (I am mainly working with C# :cool:)? 2. How can line wrapping help with horizontal scrolling? :doh: Regards Thomas
Thomas Weller wrote:
What the heck is the RAII idiom in C++
Resource Acquisition is Initialization[^] (a horrible name, but a very useful idiom)
Thomas Weller wrote:
How can line wrapping help with horizontal scrolling?
:confused: So what does it help with then? Try turning on line wrapping in Notepad and start typing - no matter what you do, there will be no horizontal scroll bars
-
if ()
{
if ()
{
if ()
{
if ()
{
if ()
{
I Fail
to see
why
this
is any
less a
horror
becaus
e it
was
line
wrappe
d
automa
ticall
y.
}
else
{} } else { } } else { }}
else
{}
}
else
{}
Today's lesson is brought to you by the word "niggardly". Remember kids, don't attribute to racism what can be explained by Scandinavian language roots. -- Robert Royall
:) And the alternative:
Acquire1();
if (!Works1())
{
Release1();
return;
}Acquire2();
if (!Works2())
{
Release1();
Release2();
return;
}Acquire3();
if (!Works3())
{
Release1();
Release2();
Release3();
return;
}...
AcquireN();
if (!WorksN())
{
Release1();
Release2();
Release3();
...
ReleaseN();
return;
}Forget to copy one of the
Releasefunctions and you have a nice resource leak :) -
:) And the alternative:
Acquire1();
if (!Works1())
{
Release1();
return;
}Acquire2();
if (!Works2())
{
Release1();
Release2();
return;
}Acquire3();
if (!Works3())
{
Release1();
Release2();
Release3();
return;
}...
AcquireN();
if (!WorksN())
{
Release1();
Release2();
Release3();
...
ReleaseN();
return;
}Forget to copy one of the
Releasefunctions and you have a nice resource leak :)In C# you can make 1-N classes, put the release code in the destructors and have it cleaned up automatically. Alternately you could have a finally block with a series of
if (Thing1.Aquired) Thing1.Release()statements.Today's lesson is brought to you by the word "niggardly". Remember kids, don't attribute to racism what can be explained by Scandinavian language roots. -- Robert Royall
-
Thomas Weller wrote:
What the heck is the RAII idiom in C++
Resource Acquisition is Initialization[^] (a horrible name, but a very useful idiom)
Thomas Weller wrote:
How can line wrapping help with horizontal scrolling?
:confused: So what does it help with then? Try turning on line wrapping in Notepad and start typing - no matter what you do, there will be no horizontal scroll bars
Nemanja Trifunovic wrote:
So what does it help with then?
Sorry, I was confusing line wrapping with the expand/collapse region feature. If you said word wrapping instead it would have been clear to me what you mean. I think this is because I am a German and not used to the exact idioms you are using in the U.S. This is a good example for a misunderstanding that would have been resolved within seconds in a face-to-face situation... :^) Regards Thomas
-
In C# you can make 1-N classes, put the release code in the destructors and have it cleaned up automatically. Alternately you could have a finally block with a series of
if (Thing1.Aquired) Thing1.Release()statements.Today's lesson is brought to you by the word "niggardly". Remember kids, don't attribute to racism what can be explained by Scandinavian language roots. -- Robert Royall
As I said, ideally you would use exceptions and RAII (in C# that would be
using) and then the code would simply look like:{
Resource1 r1;
r1.DoSomething1();Resource2 r2;
r2.DoSomething2();...
} // all resources are cleaned up here - exceptions or notMy point is that if we need to stick to the C-style error handling it is much safer to write structured code with one entry and one exit, and no copy-paste blocks. BTW, the number of
ifs is the same in both styles - the difference is how you structure them. -
Recently, I stumbled across this little gem. I don't have the exact code handy, but the gist of it is:
nErrorCode = cFtpConn.SetHost(HOST);
if (nErrorCode == 0)
{
nErrorCode = cFtpConn.SetUser(USERNAME);if (nErrorCode == 0)
{
nErrorCode = cFtpConn.SetPassword(PASSWORD);if (nErrorCode == 0) { nErrorCode = cFtpConn.SetPath(PATH); if (nErrorCode == 0) { nErrorCode = cFtpConn.SetFilename(FILENAME); if (nErrorCode == 0) { // Retrieve files, adding a few _more_ levels of `if` } else { Log("Error setting filename"); } } else { Log("Error setting path"); } } else { Log("Error setting password"); }}
else
{
Log("Error setting username");
}
}
else
{
Log("Error setting host");
} -
Thats a messy "spaghetti code". It looks very strange or like C#. X| I would write more compact functions like LogOnHost(host,user,password) and SetFilePath(path,file)
Greetings from Germany
KarstenK wrote:
It looks very strange or like C#.
What's your matter with C#? It allows for (and encourages) the cleanest and well structured code ever. :confused: Regards Thomas
-
KarstenK wrote:
It looks very strange or like C#.
What's your matter with C#? It allows for (and encourages) the cleanest and well structured code ever. :confused: Regards Thomas
-
But it allows -as seens- also the opposite. Strange is in the above code alway one Set-Function for User and Password. I hope the Ftp-objects isnt a C# class. X|
Greetings from Germany
KarstenK wrote:
But it allows -as seens- also the opposite.
Sure, it allows for bad coding style as well - as every other programming language does even more. Do you really blame C# for being a language that does not impose on its user what to say with it?? Do you have a car that forces you to adhere to traffic rules? Does your money tell you what to buy with it? ... :wtf: Regards Thomas
-
Let's assume it's C++. I consider sth. like the code above generally bad coding style. There is far to much nesting here. Supposed that most of the programmers (at least the ones I know, including myself) make an indentation of four spaces (not only two as in the 'sample'), you would quickly run out of monitor space... I would suggest a kind of 'waterfall style' coding here:
if ((nErrorCode = cFtpConn.SetHost(HOST)) != 0)
{
Log(...);
return;
}if ((nErrorCode = ...
{
Log(...);
return;
}...
This is also not perfect since it introduces many returns, but it improves the readability of the code and the return conditions are trivial and repetitive.
PIEBALDconsult wrote:
If it's C#, the methods should probably throw Exceptions.
Agreed. In a perfect world, C# - Methods would always throw exceptions and never signal an error by means of a return value. (As long as this is affordable in terms of performance). Regards Thomas
modified on Monday, November 3, 2008 4:55 AM
This is also not perfect since it introduces many returns, but it improves the readability of the code and the return conditions are trivial and repetitive.
How about:
if ((err = action1()) != 0)
log_error1();
else if ((err = action2()) != 0)
log_error2();
else if ((err = action3()) != 0)
log_error3();or
do
{
if ((err = action1()) != 0)
{log_error1(); break;}
if ((err = action2()) != 0)
{log_error2(); break;}
prepare_for_action3();
if ((err = action3()) != 0)
{log_error3(); break;}
...
} while(0); /* Loop used to allow break */The former style is nicer if each action is a single function. If stuff is required between the actions, the second approach may be helpful.
-
This is also not perfect since it introduces many returns, but it improves the readability of the code and the return conditions are trivial and repetitive.
How about:
if ((err = action1()) != 0)
log_error1();
else if ((err = action2()) != 0)
log_error2();
else if ((err = action3()) != 0)
log_error3();or
do
{
if ((err = action1()) != 0)
{log_error1(); break;}
if ((err = action2()) != 0)
{log_error2(); break;}
prepare_for_action3();
if ((err = action3()) != 0)
{log_error3(); break;}
...
} while(0); /* Loop used to allow break */The former style is nicer if each action is a single function. If stuff is required between the actions, the second approach may be helpful.
Hmm, I fear I'm not very happy with that either. The first example is just not working because due to
elsethe branches below the first one are simply unreachable, no matter what the outcome ofaction1()may be. The second alternative is just replacingreturn;withbreak;. Furthermore, it introduces a hardcoded boolean expression which always evaluates to the same value. This in my opinion is not very desirable in itself. Sure, in the example things are very easy to understand, but imagine a real life example where things can become much more complicated... Regards Thomas_Programming today is a race between software engineers striving to build bigger and better idiot-proof programs, and the Universe trying to produce bigger and better idiots. So far, the Universe is winning.
Programmer - an organism that turns coffee into software._
-
Hmm, I fear I'm not very happy with that either. The first example is just not working because due to
elsethe branches below the first one are simply unreachable, no matter what the outcome ofaction1()may be. The second alternative is just replacingreturn;withbreak;. Furthermore, it introduces a hardcoded boolean expression which always evaluates to the same value. This in my opinion is not very desirable in itself. Sure, in the example things are very easy to understand, but imagine a real life example where things can become much more complicated... Regards Thomas_Programming today is a race between software engineers striving to build bigger and better idiot-proof programs, and the Universe trying to produce bigger and better idiots. So far, the Universe is winning.
Programmer - an organism that turns coffee into software._
Thomas Weller wrote:
The first example is just not working
Sorry for that - of course it is working. :doh: My brain must be on vacation or something. Thus this indeed is a viable alternative. Regards Thomas
_Programming today is a race between software engineers striving to build bigger and better idiot-proof programs, and the Universe trying to produce bigger and better idiots. So far, the Universe is winning.
Programmer - an organism that turns coffee into software._
-
Hmm, I fear I'm not very happy with that either. The first example is just not working because due to
elsethe branches below the first one are simply unreachable, no matter what the outcome ofaction1()may be. The second alternative is just replacingreturn;withbreak;. Furthermore, it introduces a hardcoded boolean expression which always evaluates to the same value. This in my opinion is not very desirable in itself. Sure, in the example things are very easy to understand, but imagine a real life example where things can become much more complicated... Regards Thomas_Programming today is a race between software engineers striving to build bigger and better idiot-proof programs, and the Universe trying to produce bigger and better idiots. So far, the Universe is winning.
Programmer - an organism that turns coffee into software._
Thomas Weller wrote:
The second alternative is just replacing return; with break;. Furthermore, it introduces a hardcoded boolean expression which always evaluates to the same value. This in my opinion is not very desirable in itself.
Replacing a return with a break may be useful if the code has to do something besides totally exit a function. If the routine opened a file at the beginning, for example, I would consider doing a break and then closing the file after the 'while' to be much cleaner than doing "close(theFile); return;" in each failure case. It's a little irksome having a hard-coded boolean constant like that, but C does not provide any other block structure whose semantics are "run once, but be able to jump to the beginning or end." I would consider "do ... while(0);" and "do ... while(1);" to be cleaner than a "goto", at least in cases where the enclosing block does not contain any case labels. If a certain amount of code will be common to several case handlers, it would be far better to do something like:
switch(foo)
{
case 0:
code_0_special();
COMMON:
common_to_code_0_1_3();
break;
case 1:
code_1_special();
goto COMMON;
case 2:
code_2_special();
break;
case 3:
code_3_special();
goto COMMON;
default:
handle_default();
}than
switch(foo)
{
case 0:
code_0_special();
do {
common_to_code_0_1_3();
break;
case 1:
code_1_special();
continue;
case 2:
code_2_special();
break;
case 3:
code_3_special();
continue;
default:
handle_default();
break;
} while(1);
}or
switch(foo)
{
do
{
case 0:
code_0_special();
break;
case 1:
code_1_special();
break;
case 3:
code_3_special();
break;
} while(0);
common_to_code_0_1_3();
break;
case 2:
code_2_special();
break;
default:
handle_default();
break;
} while(1);
}The former would IMHO be an appropriate use of "goto"; the second is just plain horrible. The third isn't quite so bad, but is IMHO less clear than the goto.
-
I used to have a co-worker who did almost the same It went a bit along the lines off this: (sorry for the vb code but well I code in it :) )
Dim rc as integer=0
rc = doSomething()
if rc <> 0 then goto Errormessage 'yeah a gotorc= = dosomethingelse()
if rc <> 0 then goto Errormessage
... ' went on and on like this for about 200 lines
Errormessage:
Select case rc
case 1
messagebox.show("...")
case 2
messagebox.show("...")
...
end selectI true nightmare to debug but it wasn't even the worst thing I saw in his code. O and I should mention that we had/have a specific way to handle errors and the messages that should go with them. Needless to say this isn't that way :) , he just choose to ignore everything we(manly my boss (and his)) told him and just do his own thing, he didn't last very long. If I have the time I'll post some of the horrors I'v seen in it
I don't see the problem here (I assume this is VB 6?). It's a linear sequential pattern. If you think about it it's semantically equivalent to a try-catch block.
Kevin
-
If the exceptions are forbidden for whatever reason, there is nothing wrong with it.
I agree. I generally prefer to see the normal paths first followed by the error paths. Deep nesting should be avoided other things being equal. But if there is a regular pattern to the code (as in your COM example elsewhere in the thread) this is fine. What is not fine is when you have deep nesting combined with loops and randomly sprinkled if-then-else blocks.
Kevin
-
I don't see the problem here (I assume this is VB 6?). It's a linear sequential pattern. If you think about it it's semantically equivalent to a try-catch block.
Kevin
If it was VB6 is would agree but this code was written in .NET and only about a year ago (with visual studio 2005)
-
If it was VB6 is would agree but this code was written in .NET and only about a year ago (with visual studio 2005)
OK, in that case he should be shot. :laugh: BTW, why did they keep that stuff in VB .NET? Ditto Option Explicit turned off by default?
Kevin
-
OK, in that case he should be shot. :laugh: BTW, why did they keep that stuff in VB .NET? Ditto Option Explicit turned off by default?
Kevin
Yeah and that wasn't even his biggest horror :sigh: I'd laugh if I didn't have to work in the code :sigh: