The Exception (Mis)handling Circus Continues!
-
Virtually every one of the approximately 1000 try/catch/finally blocks in my project simply re-throws any general exception caught. That's bad enough... but this little piece of work really caught my attention. This is something really special.
Try
<font color="GREEN">' SNIP
</font> <font color="GREEN">' tons of code that could throw many varied and unusual exceptions ...
</font> <font color="GREEN">' SNIP
</font>
Catch exRange <font color="RED">As</font> System<font color="BLUE" size="+1">.</font>IndexOutOfRangeException
Throw exRange
<font color="RED">Return</font> <font color="RED">String</font><font color="BLUE" size="+1">.</font><font color="RED">Empty</font>Catch ex <font color="RED">As</font> Exception
Throw ex
<font color="RED">Return</font> <font color="RED">String</font><font color="BLUE" size="+1">.</font><font color="RED">Empty</font>Finally
<font color="RED">End</font> Try
-
I don't think the original writers wrote it like that. Maybe they had Return String.Empty in there originaly and added the Throws. They should have deleted the return lines. I try to not have too many returns in my code, just declare a string up top and return the result at the end. If you're that worried about it, set that declared string to string.Empty before throwing the exception. Even though it is completely unnecessary because it'll never happen anyway.
-
@pitchingchris said:
I don't think the original writers wrote it like that.
You might not think so, but thanks to the magic of source control, I can know for a fact that they did. At least, I know that the original author of this code added it to the repository as-is 3 years ago.
But even ignoring the return-after-throw pattern (there's gotta be a better name for it) there are numerous other WTFs.
-
@pitchingchris said:
I don't think the original writers wrote it like that. Maybe they had Return String.Empty in there originaly and added the Throws. They should have deleted the return lines. I try to not have too many returns in my code, just declare a string up top and return the result at the end. If you're that worried about it, set that declared string to string.Empty before throwing the exception. Even though it is completely unnecessary because it'll never happen anyway.
Wow, you definitely missed a whole bunch of WTFs... Now, be honest, did you write this code??
-
@MasterPlanSoftware said:
Wow, you definitely missed a whole bunch of WTFs... Now, be honest, did you write this code??
Actually, to be fair, the red highlighted statements were not my doing (the code->html software did it) and are probably why he focused on them.
-
@djork said:
@pitchingchris said:
I don't think the original writers wrote it like that.
You might not think so, but thanks to the magic of source control, I can know for a fact that they did. At least, I know that the original author of this code added it to the repository as-is 3 years ago.
But even ignoring the return-after-throw pattern (there's gotta be a better name for it) there are numerous other WTFs.
maybe the wrote it without the Throws, and modified it again *before* they checked it in.
In which case they were just lazy.
-
I once used an IDE (possibly Eclipse?) that complained if there was no return statement, even if there was an exception, so I used to have a lot of "return null" lines after a throw. Perhaps that explains that bit of it?
Now, moving onto why he's treating IndexOutOfRangeException differently, I now open up the floor for discussion.
-
@djork said:
Actually, to be fair, the red highlighted statements were not my doing (the code->html software did it) and are probably why he focused on them.
That's a rather crappy converter anyway — it didn't even handle all the exception-handling-related keywords and for some curious reason highlighted Empty.
-
@MasterPlanSoftware said:
[Wow, you definitely missed a whole bunch of WTFs... Now, be honest, did you write this code??
Sorry, my vb.Net skills are rather rusty. I've been doing so much C++ and C# programming, its hard to remember all the syntax rules. And parts of the function are missing, so its hard to infer what you can't see.
-
@pitchingchris said:
@MasterPlanSoftware said:
[Wow, you definitely missed a whole bunch of WTFs... Now, be honest, did you write this code??
Sorry, my vb.Net skills are rather rusty. I've been doing so much C++ and C# programming, its hard to remember all the syntax rules. And parts of the function are missing, so its hard to infer what you can't see.
OK then, I'll help.
1) Two different kinds of exceptions are handled in the exact same way ...
2) ... by re-throwing them
3) A Finally clause could [i]actually[/i] be used to return a value, but is empty in this case
-
@djork said:
@pitchingchris said:
@MasterPlanSoftware said:
[Wow, you definitely missed a whole bunch of WTFs... Now, be honest, did you write this code??
Sorry, my vb.Net skills are rather rusty. I've been doing so much C++ and C# programming, its hard to remember all the syntax rules. And parts of the function are missing, so its hard to infer what you can't see.
OK then, I'll help.
1) Two different kinds of exceptions are handled in the exact same way ...
2) ... by re-throwing them
3) A Finally clause could [i]actually[/i] be used to return a value, but is empty in this case
Will the finally be executed even if they do a throw from the catch blocks ? I don't think it would. It would still be thrown if an exception was handled, but I don't think it would if you throw another one.
-
@pitchingchris said:
Will the finally be executed even if they do a throw from the catch blocks ? I don't think it would. It would still be thrown if an exception was handled, but I don't think it would if you throw another one.
I'm not certain... I'd have to try it... but I do know that if you return inside of a Catch (in VB) the Finally still executes. This pattern is used a lot in the project I'm on.
-
@djork said:
@pitchingchris said:
Will the finally be executed even if they do a throw from the catch blocks ? I don't think it would. It would still be thrown if an exception was handled, but I don't think it would if you throw another one.
I'm not certain... I'd have to try it... but I do know that if you return inside of a Catch (in VB) the Finally still executes. This pattern is used a lot in the project I'm on.
Given that the whole purpose of "finally" is to make sure your clean up code is run IN ANY CASE, wouldn't it make the whole thing kinda pointless if there were situations where it didn't run?
-
@pitchingchris said:
@MasterPlanSoftware said:
[Wow, you definitely missed a whole bunch of WTFs... Now, be honest, did you write this code??
Sorry, my vb.Net skills are rather rusty. I've been doing so much C++ and C# programming, its hard to remember all the syntax rules. And parts of the function are missing, so its hard to infer what you can't see.
There is nothing at all VB specific about exception handling. The WTFs here are not syntax related, but rather things like erasing the stack when re-throwing an exception.
So I stand by my statement, if you have been using C# and can't spot the WTF(s), then you are probably guilty as well.
-
@djork said:
3) A Finally clause could [i]actually[/i] be used to return a value, but is empty in this case
A finally clause with a return statement will do no such thing. In fact, it will do absolutely nothing, other than refuse to compile.
Why is try/catch/finally so confusing for so many people? It's really very simple: "Catch" suppresses the exception, and optionally gives you detailed information about it. "Finally" does not suppress an exception, it just delays invocation of the exception handler until after the code in the finally block has been executed (i.e. cleanup code). It's for precisely this reason that you can't ever return from a finally block - doing so would interrupt the exception handler and produce wildly unpredictable results.
-
Not to mention, the function SHOULDN'T 'return'. It is an exception... that is the very premise.
-
@rbowes said:
I once used an IDE (possibly Eclipse?) that complained if there was no return statement, even if there was an exception, so I used to have a lot of "return null" lines after a throw. Perhaps that explains that bit of it?
That seems a bit dumb, if true. Code such as the above will cause an "unreachable code" warning using the MS .NET compilers. I'd be very annoyed if my IDE forced me to put in code that a) will be compiled out anyway and b) actually caused a compiler warning.
-
@Nozz said:
@rbowes said:
I once used an IDE (possibly Eclipse?) that complained if there was no return statement, even if there was an exception, so I used to have a lot of "return null" lines after a throw. Perhaps that explains that bit of it?
That seems a bit dumb, if true. Code such as the above will cause an "unreachable code" warning using the MS .NET compilers. I'd be very annoyed if my IDE forced me to put in code that a) will be compiled out anyway and b) actually caused a compiler warning.
Well every function that is supposed to return something is supposed to do that. Sometimes it will throw an exception, but if everything works, it should return a value. I hope you're not suggesting that the IDE allow something like this:
public int foo(int a)
{
try {
/-- code that could cause an error --/
}
catch (Exception)
{ /-- what to do with exception --/}
finally
{ /-- whatever --/ }
// I don't have to return anything because I have an error.
}
-
upon closer inspection, the exception block is completely pointless as they are not throwing a different exception, but the same one. So they might has well hav just put the code without the try and caught it at a higher level to acheive the same result. Now, if they were throwing a different exception (maybe a custom one) than there wouldn't be much wrong with the code minus the unnecessary returns. I normally don't follow the try/catch/rethrow scenario, but it is mentioned in multiple sources (I know how some people feel about books, based on a sidebar thread this week). It could be used in situations where you want the code to purposely bail out a long task that requires each previous one to succeed.
-
@Aaron said:
Why is try/catch/finally so confusing for so many people? It's really very simple: "Catch" suppresses the exception, and optionally gives you detailed information about it. "Finally" does not suppress an exception, it just delays invocation of the exception handler until after the code in the finally block has been executed (i.e. cleanup code). It's for precisely this reason that you can't ever return from a finally block - doing so would interrupt the exception handler and produce wildly unpredictable results.
I think you got that one wrong.. The finally block would execute after the exception handler (the catch). A good example would to be if you had a file or database open and you hit an exception, you can catch it (and maybe rollback your changes) before you close the file or database in the Finally block. You are right about the return though, and most compilers should prevent you from doing this.
-
@belgariontheking said:
Well every function that is supposed to return something is supposed to do that. Sometimes it will throw an exception, but if everything works, it should return a value. I hope you're not suggesting that the IDE allow something like this:
public int foo(int a)
{
try {
/-- code that could cause an error --/
}
catch (Exception)
{ /-- what to do with exception --/}
finally
{ /-- whatever --/ }
// I don't have to return anything because I have an error.
}No, that's not what I said. He said he that he has return statements a throw, not after a catch. Therefore, I'm suggesting that the IDE allows this:
public object foo (int a)
{
throw new Exception("Blah");
}His IDE would force him to do this:
public object foo (int a)
{
throw new Exception("Blah");
return null;
}
Which would actually cause a compiler warning (in C#, anyway).
-
@Nozz said:
@belgariontheking said:
Well every function that is supposed to return something is supposed to do that. Sometimes it will throw an exception, but if everything works, it should return a value. I hope you're not suggesting that the IDE allow something like this:
public int foo(int a)
{
try {
/-- code that could cause an error --/
}
catch (Exception)
{ /-- what to do with exception --/}
finally
{ /-- whatever --/ }
// I don't have to return anything because I have an error.
}No, that's not what I said. He said he that he has return statements a throw, not after a catch. Therefore, I'm suggesting that the IDE allows this:
public object foo (int a)
{
throw new Exception("Blah");
}His IDE would force him to do this:
public object foo (int a)
{
throw new Exception("Blah");
return null;
}
Which would actually cause a compiler warning (in C#, anyway).I would kind of hope it returns a warning... who would write code like that?
If you have a function it has to return a value on a code path. That is what the warning says.
Why would you write a function that only throws an exception?
-
@MasterPlanSoftware said:
I would kind of hope it returns a warning... who would write code like that?
If you have a function it has to return a value on a code path. That is what the warning says.
Why would you write a function that only throws an exception?
Who would write code like that? The guy whose post I responded to in the beginning, because his IDE enforced it.
The post I was responding to said that his IDE forced him to have a return statement immediately after a throw statement. If an exception is thrown then he should NOT need to return a value on THAT path, and the compiler would raise a warning if he does, yet his IDE whinged if he DIDN'T return something on that path. Read the post I initially quoted, you'll see what I'm talking about.
Obviously you would never write a function that only throws an exception. It was just a very rudimentry example to prove a point.
-
@Nozz said:
@MasterPlanSoftware said:
I would kind of hope it returns a warning... who would write code like that?
If you have a function it has to return a value on a code path. That is what the warning says.
Why would you write a function that only throws an exception?
Who would write code like that? The guy whose post I responded to in the beginning, because his IDE enforced it.
The post I was responding to said that his IDE forced him to have a return statement immediately after a throw statement. If an exception is thrown then he should NOT need to return a value on THAT path, and the compiler would raise a warning if he does, yet his IDE whinged if he DIDN'T return something on that path. Read the post I initially quoted, you'll see what I'm talking about.
Obviously you would never write a function that only throws an exception. It was just a very rudimentry example to prove a point.
(Just tested for my own sanity in VS2008)
This (completely worthless code):
private object whoha()
{
try
{
return new object();
}
catch (Exception ex)
{
throw;
}
}Will not warn you about returning a value.
This:
private object whoha()
{
try
{
int A = 0;A++;
}
catch (Exception ex)
{
throw;
}
}Will return "Error .whoha()': not all code paths return a value"
Explain to me what is the problem?
The IDE is not the problem here. The person who wrote the code doesn't understand exceptions clearly.
-
@MasterPlanSoftware said:
@Nozz said:
@MasterPlanSoftware said:
I would kind of hope it returns a warning... who would write code like that?
If you have a function it has to return a value on a code path. That is what the warning says.
Why would you write a function that only throws an exception?
Who would write code like that? The guy whose post I responded to in the beginning, because his IDE enforced it.
The post I was responding to said that his IDE forced him to have a return statement immediately after a throw statement. If an exception is thrown then he should NOT need to return a value on THAT path, and the compiler would raise a warning if he does, yet his IDE whinged if he DIDN'T return something on that path. Read the post I initially quoted, you'll see what I'm talking about.
Obviously you would never write a function that only throws an exception. It was just a very rudimentry example to prove a point.
(Just tested for my own sanity in VS2008)
This (completely worthless code): [...] Will not warn you about returning a value.
This: [...]
Will return "Error .whoha()': not all code paths return a value"
Mate, you really are preaching to the converted here, and actually missing the point completely. Everything you've said there is in complete accordance with what I have said. I get how exceptions work, and when are when not values should be returned. This is the post from rbowes that spawned my responses:
I once used an IDE that complained if there was no
return statement, even if there was an exception, so I used to have a
lot of "return null" lines after a throw.See that bit right there where he said he puts "return null" directly after a "throw" because his IDE complained if he didn't? All I'm saying is that doing that WILL cause a compiler warning.
-
@MasterPlanSoftware said:
@Nozz said:
No, that's not what I said. He said he that he has return statements a throw, not after a catch. Therefore, I'm suggesting that the IDE allows this:
public object foo (int a)
{
throw new Exception("Blah");
}His IDE would force him to do this:
public object foo (int a)
Which would actually cause a compiler warning (in C#, anyway).
{
throw new Exception("Blah");
return null;
}I would kind of hope it returns a warning... who would write code like that?
If you have a function it has to return a value on a code path. That is what the warning says.
Why would you write a function that only throws an exception?
It happens more often than you might think in large frameworks with multiple intercommunicating components relying on contracts through interfaces:
throw new Exception("This member has not been implemented yet.");
-
@Ragnax said:
@MasterPlanSoftware said:
@Nozz said:
No, that's not what I said. He said he that he has return statements a throw, not after a catch. Therefore, I'm suggesting that the IDE allows this:
public object foo (int a)
{
throw new Exception("Blah");
}His IDE would force him to do this:
public object foo (int a)
Which would actually cause a compiler warning (in C#, anyway).
{
throw new Exception("Blah");
return null;
}I would kind of hope it returns a warning... who would write code like that?
If you have a function it has to return a value on a code path. That is what the warning says.
Why would you write a function that only throws an exception?
It happens more often than you might think in large frameworks with multiple intercommunicating components relying on contracts through interfaces:
throw new Exception("This member has not been implemented yet.");
Your example wouldn't fit this argument. Who would care about the validity of a return statement in a function that ONLY throws a not implemented exception?
-
@MasterPlanSoftware said:
Who would care about the validity of a return statement in a function that ONLY throws a not implemented exception?
Somebody who knew that the function would eventually be implemented, and didn't want to have to put off writing their code until that happened. Did you not read the first sentence of Ragnax's post?
-
@NSCoder said:
@MasterPlanSoftware said:
Who would care about the validity of a return statement in a function that ONLY throws a not implemented exception?
Somebody who knew that the function would eventually be implemented, and didn't want to have to put off writing their code until that happened. Did you not read the first sentence of Ragnax's post?You didn't read my post either. I am arguing who would care about a stray return statement in code that was not yet implemented.
I realize why functions are included that throw not implemented exceptions.... But thanks for the enlightenment.
