|
He probably was told to use try-catch so he did, how could that be wrong? 
I have seen this kind of thing from people who aren't used to try-catch and/or are just putting it in the code because someone told them to do it. Of course, if you want the try-catch to be useful for tracking down issues in the code then having a huge block where there could be dozens of points of failure and not catching the individual types of exceptions is not the way to go.
Bill W
Just because code works, it doesn't mean that it is good code.
|
|
|
|
|
Once, many moons ago, I got talked to by another developer because I wasn't using try catch blocks to do error handling. I checked and validated results as I went, using if/elses. No, it wasn't particularly elegant and I would do it differently now but it worked and handled failures well.
A few days later I looked at some of his code to get an idea for how to structure it using try/catch and came upon something very much like what you posted. I was horrified, but because I knew try/catch was all the rage, and I was new, I thought his was more correct and didn't say anything. The company went out of business a few months later.
(Although, to be fair, that was more likely a result of us having free arcade games in the office and spur of the moment Quake tournaments.)
It was a long time before I realized the true benefits of properly structured exception handling, thanks to some early misguidance. I'm still appalled at how it gets frequently abused though.
He said, "Boy I'm just old and lonely,
But thank you for your concern,
Here's wishing you a Happy New Year."
I wished him one back in return.
|
|
|
|
|
I would not have minded if he made use of if/else statements or at least something to handle or track issues, but there was not of that, just a load of messy code! I found earlier this year, done by the same guy, some code which as a HUGE for-loop containing dozens of if/else statements to calculate values for paging data, when I was able to replace it all with one single line of math...
I could understand if this guy was a junior, but he was a "professional contractor"!
|
|
|
|
|
Professional contractors follow Sturgeon's Law closely, except that the percentage is more likely to be 98% in my experience.
|
|
|
|
|
David Kentley wrote: I'm still appalled at how it gets frequently abused though.
try
{
// some code
}
catch {}
is quite common. 
There can be some cases in which such a pattern is fine, though usually you should be catching a specific exception, but you should always explain why/what you're doing in such cases.
Kevin
|
|
|
|
|
Kevin McFarlane wrote: There can be some cases in which such a pattern is fine, though usually you should be catching a specific exception, but you should always explain why/what you're doing in such cases.
I wish .net included a few more 'tryDoSomething' methods. For example, if one thread has just changed some information that should be displayed be a control, a sensible thing to do is to BeginInvoke the control's update handler (preferably using a 'Threading.Interlocked.CompareExchange'ed flag to prevent excessive 'BeginInvoke's). Unfortunately, I know of no good way to avoid the risk of the control being disposed before the BeginInvoke. In a situation like that, a tryBeginInvoke would be extremely handy; if it works, great. If the control's been disposed or otherwise lacks a windowing context, there's no need to update it and the call may be safely ignored.
If a try block contains a single BeginInvoke and the catch block is empty, is that not sufficiently clear "Try this, and if it works great--if not, meh."
|
|
|
|
|
David Kentley wrote: I checked and validated results as I went, using if/elses.
I'm a fan of doing some error trapping this way. It's quick and you can log a very specific error message that says exactly what's wrong, like an object doesn't exist. That's good for the developer that has to fix the issue. Besides, having an exception thrown and being caught is expensive from a processor time point of view.
I've also seen large blocks of code wrapped in a try catch block where the catch block logged an "unhandled" exception. Might as well have said "s&*t happened"!
|
|
|
|
|
void(entity targ, entity attacker) ClientObituary =
{
local float rnum;
local string deathstring, deathstring2;
rnum = random();
...
if (targ.classname == "player")
{
if (attacker.classname == "player")
{
if (targ == attacker)
{
attacker.frags = attacker.frags - 1;
bprint (targ.netname);
if (targ.weapon == 64 && targ.waterlevel > 1)
{
bprint (" discharges into the water.\n");
return;
}
if (targ.weapon == IT_GRENADE_LAUNCHER)
bprint (" tries to put the pin back in\n");
else
bprint (" becomes bored with life\n");
return;
}
else
{
attacker.frags = attacker.frags + 1;
rnum = attacker.weapon;
if (rnum == IT_AXE)
{
deathstring = " was ax-murdered by ";
deathstring2 = "\n";
}
...
bprint (targ.netname);
bprint (deathstring);
bprint (attacker.netname);
bprint (deathstring2);
}
return;
}
else
{
targ.frags = targ.frags - 1;
bprint (targ.netname);
if (attacker.flags & FL_MONSTER)
{
if (attacker.classname == "monster_army")
bprint (" was shot by a Grunt\n");
if (attacker.classname == "monster_demon1")
bprint (" was eviscerated by a Fiend\n");
...
return;
}
if (attacker.classname == "explo_box")
{
bprint (" blew up\n");
return;
}
if (attacker.solid == SOLID_BSP && attacker != world)
{
bprint (" was squished\n");
return;
}
...
if (targ.deathtype == "falling")
{
targ.deathtype = "";
bprint (" fell to his death\n");
return;
}
bprint (" died\n");
The whole routine is about 250 lines long, but for whatever reason I particularly like the last comment.
|
|
|
|
|
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)
{
}
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?
|
|
|
|
|
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 nErrorCode is non-zero the assignment and comparision are never done.
Sorted!
Panic, Chaos, Destruction.
My work here is done.
|
|
|
|
|
|
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/finally approach or implementing sort of Dispose pattern - depending on the problem to solve and whether it is C/C++ or C#.
I would definitely not end up with a bunch of ifs in this case, but the 'sample code' does not give any hint in that direction - and I think this is not the point here... 
Regards
Thomas
|
|
|
|
|
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 pattern is in C#...
In my opinion, there are two problems with code consisting of many nested if paths:
- 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 many ifs are a coding horror in my opinion: Readability and maintainability issues.
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 if part, and the error handling in the else part. 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? 
|
|
|
|
|
|
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?
 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
|
|
|
|
|
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. 
Regards
Thomas
|
|
|
|
|
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 Release functions 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
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
)?