A big if
-
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");
}C/C++ or C#? If it's C#, the methods should probably throw Exceptions. If it's C/C++, I see no real problem with it. Had I written it, it would be:
if ((nErrorCode = cFtpConn.SetHost(HOST)) == 0)and the Log messages would include the value of nErrorCode. How would you improve it? -
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");
}:omg:
"The clue train passed his station without stopping." - John Simmons / outlaw programmer "Real programmers just throw a bunch of 1s and 0s at the computer to see what sticks" - Pete O'Hanlon "Not only do you continue to babble nonsense, you can't even correctly remember the nonsense you babbled just minutes ago." - Rob Graham
-
C/C++ or C#? If it's C#, the methods should probably throw Exceptions. If it's C/C++, I see no real problem with it. Had I written it, it would be:
if ((nErrorCode = cFtpConn.SetHost(HOST)) == 0)and the Log messages would include the value of nErrorCode. How would you improve it?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
-
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
If you can't throw exceptions or the return type is already taken, there is another approach:
if ((nErrorCode = cFtpConn.SetHost(HOST)) != 0)
{
Log("Error setting host");
}if (nErrorCode == 0 && (nErrorCode = cFtpConn.SetUser(USERNAME)) !=0)
{
Log("Error setting username");
}if (nErrorCode == 0 && (nErrorCode = cFtpConn.SetPassword(PASSWORD)) != 0)
{
Log("Error setting password");
}Once
nErrorCodeis non-zero the assignment and comparision are never done. Sorted!
Panic, Chaos, Destruction. My work here is done.
-
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");
}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
-
If you can't throw exceptions or the return type is already taken, there is another approach:
if ((nErrorCode = cFtpConn.SetHost(HOST)) != 0)
{
Log("Error setting host");
}if (nErrorCode == 0 && (nErrorCode = cFtpConn.SetUser(USERNAME)) !=0)
{
Log("Error setting username");
}if (nErrorCode == 0 && (nErrorCode = cFtpConn.SetPassword(PASSWORD)) != 0)
{
Log("Error setting password");
}Once
nErrorCodeis non-zero the assignment and comparision are never done. Sorted!
Panic, Chaos, Destruction. My work here is done.
Yep, that's good too.
-
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
In fact, I think your approach is more problematic. What if resources need to be released before returning from the function? The if branch will grow bigger and bigger as you are nearing the end of the function, and most of it will be a copy-paste code.
-
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");
}If the exceptions are forbidden for whatever reason, there is nothing wrong with it.
-
In fact, I think your approach is more problematic. What if resources need to be released before returning from the function? The if branch will grow bigger and bigger as you are nearing the end of the function, and most of it will be a copy-paste code.
Nemanja Trifunovic wrote:
What if resources need to be released before returning from the function? The if branch will grow bigger and bigger as you are nearing the end of the function
True, but if there are some actions to be taken that are not part of the functional code (e.g. releasing resources), I'd consider to take a
try/finallyapproach or implementing sort ofDispose pattern- depending on the problem to solve and whether it is C/C++ or C#. I would definitely not end up with a bunch ofifs in this case, but the 'sample code' does not give any hint in that direction - and I think this is not the point here... :rolleyes: Regards Thomas -
If the exceptions are forbidden for whatever reason, there is nothing wrong with it.
Nemanja Trifunovic wrote:
there is nothing wrong with it
As always with coding style, it's to some extent a matter of taste and in parts the result of former - mostly painful - experiences. And sometimes it may simply depend on the level of agreement one can achieve within a project team :sigh:. Here's what I think about it: http://www.codeproject.com/Feature/CodingHorrors.aspx?fid=392254&select=2790521&fr=1#xx2790521xx[^] Regards Thomas
-
Nemanja Trifunovic wrote:
What if resources need to be released before returning from the function? The if branch will grow bigger and bigger as you are nearing the end of the function
True, but if there are some actions to be taken that are not part of the functional code (e.g. releasing resources), I'd consider to take a
try/finallyapproach or implementing sort ofDispose pattern- depending on the problem to solve and whether it is C/C++ or C#. I would definitely not end up with a bunch ofifs in this case, but the 'sample code' does not give any hint in that direction - and I think this is not the point here... :rolleyes: Regards ThomasThomas Weller wrote:
True, but if there are some actions to be taken that are not part of the functional code (e.g. releasing resources), I'd consider to take a try/finally approach or implementing sort of Dispose pattern - depending on the problem to solve and whether it is C/C++ or C#.
Exceptions coupled with automatic release of resources are the best approach. But if this approach is not available, the nested
ifs still work well and are reasonably readable: at least the error paths are somewhat separated from the "normal" flow Typical well written COM code often looks like:ISomeInterface* pInter(NULL);
HRESULT hr = E_FAIL;
if (SUCCEEDED(SomeFactory->CreateSomeObject(&pInter)))
{
if (SUCCEEDED(pInter->Operation1()))
{
if (SUCCEEDED(pInter->Operation2()))
{
DoSomething(pInter);
hr = S_OK;
}
else
hr = E_WHATEVER2;
else
hr = E_WHATEVER1;
}
else
pInter->Release();
}return hr;
-
Thomas Weller wrote:
True, but if there are some actions to be taken that are not part of the functional code (e.g. releasing resources), I'd consider to take a try/finally approach or implementing sort of Dispose pattern - depending on the problem to solve and whether it is C/C++ or C#.
Exceptions coupled with automatic release of resources are the best approach. But if this approach is not available, the nested
ifs still work well and are reasonably readable: at least the error paths are somewhat separated from the "normal" flow Typical well written COM code often looks like:ISomeInterface* pInter(NULL);
HRESULT hr = E_FAIL;
if (SUCCEEDED(SomeFactory->CreateSomeObject(&pInter)))
{
if (SUCCEEDED(pInter->Operation1()))
{
if (SUCCEEDED(pInter->Operation2()))
{
DoSomething(pInter);
hr = S_OK;
}
else
hr = E_WHATEVER2;
else
hr = E_WHATEVER1;
}
else
pInter->Release();
}return hr;
Nemanja Trifunovic wrote:
Exceptions coupled with automatic release of resources are the best approach.
This is a quite good definition of what
Dispose patternis in C#... :) In my opinion, there are two problems with code consisting of many nestedifpaths: - It is hard to follow if it gets lengthy. Error probability increases dramatically with every level of nesting - especially when it comes to maintenance. - 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! That's why the manyifs are a coding horror in my opinion: Readability and maintainability issues. Regards Thomas -
Nemanja Trifunovic wrote:
Exceptions coupled with automatic release of resources are the best approach.
This is a quite good definition of what
Dispose patternis in C#... :) In my opinion, there are two problems with code consisting of many nestedifpaths: - It is hard to follow if it gets lengthy. Error probability increases dramatically with every level of nesting - especially when it comes to maintenance. - 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! That's why the manyifs are a coding horror in my opinion: Readability and maintainability issues. Regards ThomasThomas 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:
-
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:
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:
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