Something Bad Happened

danixdefcon5

Projects grow big. Development cycles very often end up missing deadlines, and coding standards usually go bad. One of my favorite mishaps is when coders decide to do things like enclosing everything inside a try/catch block that catches Exception. But checking out some code that has gone through many hands, I almost laughed at this one:

                try {
                    registro = ejbTxnMgr.realizarCargos();
                } catch (RuntimeException e) {
                    logger.critical("Something bad happened", e);
                }
                  throw e;
                }
<FONT face=Arial></FONT> 
<FONT face=Arial>Give it credit to whoever did this, but at least now I know that SOMETHING BAD HAPPENED.</FONT>

PSWorx

logger.critical("Something bad happened", e);

I don't know how your logging system is designed, but it seems to me that he passes the exception on to it, so no information would be lost.

Of course that doesn't change the general brain-dead-ness of that approach.

pitchingchris

I agree with PSWorx. If your logger does output the exception information, it won't really be a WTF. Sure it could be more specific, but if he logs the Exception information correctly, it will point you to the culprit. Now if the exception information isn't getting logged, that would be another thing...

you'd just have a line in the log that says..

Something bad happened. (lame, aint it)

vt_mruhlin

try {
 registro = ejbTxnMgr.realizarCargos();
 } catch (RuntimeException e) {
 logger.critical("Something bad happened", e);
 } // <-- This is the WTF, or OP made a typo.
 throw e;
 }

"e is undefined....".

I've written so many "something bad happened" log messages in my day.

MasterPlanSoftware

I agree the code makes little or no sense, but I have a feeling this is just the OP more or less just going "Hey, I want to post a WTF too!".

DaveK1

@MasterPlanSoftware said:

I agree the code makes little or no sense, but I have a feeling this is just the OP more or less just going "Hey, I want to post a WTF too!".

Nah, waittaminnit. It's not just the typo; let's be generous and assume the "throw e" was meant to be in scope. But isn't there still some reason why you shouldn't just blindly catch and re-throw an exception you don't understand or intend to handle, anyway? I'm sure I remember reading about it in an earlier tdwtf story. Isn't it something like, because you catch it as a RuntimeException, when you re-throw it, you re-throw it as a RuntimeException and lose the information about which, if any, derived subclass it was?

bstorer

@DaveK said:

@MasterPlanSoftware said:
I agree the code makes little or no sense, but I have a feeling this is just the OP more or less just going "Hey, I want to post a WTF too!".

Nah, waittaminnit. It's not just the typo; let's be generous and assume the "throw e" was meant to be in scope. But isn't there still some reason why you shouldn't just blindly catch and re-throw an exception you don't understand or intend to handle, anyway? I'm sure I remember reading about it in an earlier tdwtf story. Isn't it something like, because you catch it as a RuntimeException, when you re-throw it, you re-throw it as a RuntimeException and lose the information about which, if any, derived subclass it was?

Depends on the language. In Java, it doesn't care how you rethrow it, or even whether you rethrow it. It'll still contain all the proper stack trace, and all the additional information of the subclass.

pitchingchris

@vt_mruhlin said:

try {
 registro = ejbTxnMgr.realizarCargos();
 } catch (RuntimeException e) {
 logger.critical("Something bad happened", e);
 } // <-- This is the WTF, or OP made a typo.
 throw e;
 }

"e is undefined....".

Must be using mono or something. C# using a Microsoft compiler shouldn't compile. e would have a duplicate definitiion in the same scope. I don't know a thing about Java..

vt_mruhlin

@bstorer said:

@DaveK said:
@MasterPlanSoftware said:
I agree the code makes little or no sense, but I have a feeling this is just the OP more or less just going "Hey, I want to post a WTF too!".

Nah, waittaminnit. It's not just the typo; let's be generous and assume the "throw e" was meant to be in scope. But isn't there still some reason why you shouldn't just blindly catch and re-throw an exception you don't understand or intend to handle, anyway? I'm sure I remember reading about it in an earlier tdwtf story. Isn't it something like, because you catch it as a RuntimeException, when you re-throw it, you re-throw it as a RuntimeException and lose the information about which, if any, derived subclass it was?

Depends on the language. In Java, it doesn't care how you rethrow it, or even whether you rethrow it. It'll still contain all the proper stack trace, and all the additional information of the subclass.

I do remember somebody talking about that. Whichever language they were referring to had an option of just saying "throw" that would throw it as the original type. Was that C++? All the C++ development I've done has been with old C coders, so the issue never really came up... But it seems like the sort of thing C++ would do.

Morbii

@vt_mruhlin said:

I do remember somebody talking about that. Whichever language they were referring to had an option of just saying "throw" that would throw it as the original type. Was that C++? All the C++ development I've done has been with old C coders, so the issue never really came up... But it seems like the sort of thing C++ would do.

Both C++ and C#, maybe more.

I'd put my virtual money on Java. "ejb" suggests Enterprise Java Beans to me.

Now, in Java you have the following class hierarchy: Throwable, subclassed by Exception, subclassed by RuntimeException.

RuntimeExceptions are exceptions that generally aren't predicted, like NullPointerExceptions or ClassCastExceptions. They are a particular kind of exception that you don't need to declare on a method's signature.

So, this code makes some sense to me. They're logging unexpectable, what-the-hell-just-happened exceptions.

PhillS

@Zecc said:

So, this code makes some sense to me. They're logging unexpectable, what-the-hell-just-happened exceptions.

Makes sense to me too. I probably wouldn't have used "Something bad happened", but still.

There can be good reasons to log something and then re-throw (or throw another exception with the cause as the exception you have just caught). Mainly to ensure that the error and stack trace gets logged - I've had occasions where some framework (JSF, I'm looking at you) I've been using has just swallowed an exception without logging anything.

AbbydonKrafts

@PhillS said:

Makes sense to me too. I probably wouldn't have used "Something bad happened", but still.

Same here. Mine writes out "Unexpected Error". That way the troubleshooters (and myself) know that the error was not forseen.

@PhillS said:

There can be good reasons to log something and then re-throw

I rarely log and re-throw. It's one or the other for me. All of my libraries simply re-throw. It's up to the application to do something with it. That makes for a beautiful stack trace. Heh.

vt_mruhlin

@AbbydonKrafts said:

I rarely log and re-throw. It's one or the other for me. All of my libraries simply re-throw. It's up to the application to do something with it. That makes for a beautiful stack trace. Heh.

I like log & throw because I never know who's going to do a catch & mangle or a catch & ignore on it above me.

Depending on how your support folks work, it could make it easier to diagnose a problem too. At my current company, they get an IM every time there's an error, then blame whichever module logged the error. So insuring that the first one to catch the exception is the first one to log it, you can actually get the bug reported to the right place.

Course at my last job they just looked at the latest error in the log files and ignored anything that happened before it, so this wouldn't help there.

DZ_Jay

@vt_mruhlin said:

I do remember somebody talking about that. Whichever language they were referring to had an option of just saying "throw" that would throw it as the original type.

That's how it works in Delphi, where "Raise" is used instead of "throw" . If you call Raise E, you re-raise the same exception object, but lose the stack trace. However, if you call just "Raise", it will re-raise the original object with all its attributes intact.

dZ.

DZ_Jay

@AbbydonKrafts said:

I rarely log and re-throw. It's one or the other for me. All of my libraries simply re-throw. It's up to the application to do something with it. That makes for a beautiful stack trace. Heh.

I do the same. Whoever ultimately handles the exception has the responsibility of logging it. If nobody handles it, there is a global catch-all at the outermost application level which logs and, if necessary, terminates the application. The idea is that only those exceptions which are truly unforseen and, er, exceptional, should make it that far up the chain, everything else should have been handled (and logged, if necessary).

The reason I don't log and re-throw is that then you may end up with duplicate entries, with potentially different messages at different points of the call stack.

Nether

I'm not sure what the big deal is. If your application-made exceptions are created with useful messages and throw to the parent for handling, and your log settings always print the exception messages anyway, then there may be nothing more to add. In fact, in log4j, I found that it won't print the stack trace unless you use the (string, Throwable) form of any logger. So, many invocations in my previous application have something to the effect of log.error("", e). This is because there is simply nothing else relevant to add to the information already in the exception. The error logging is by far the best and most informative of any application I've ever made. If something goes wrong, you know exactly, what, where, when, and why, due to a rigid standard of how logging and exception chaining is handled.

The error string itself is halfway amusing, but this isn't a WTF.

MasterPlanSoftware

@Nether said:

The error string itself is halfway amusing, but this isn't a WTF.

Rethrowing the exception and destroying the stack trace is always a WTF.

</Assumption>

Not to mention, the fact that the OP didn't even reproduce the code correctly when posting here makes it a whole different WTF.

AbbydonKrafts

@DZ-Jay said:

Whoever ultimately handles the exception has the responsibility of logging it.

I take that approach because the library doesn't know how the application is handling logging. My applications log errors and progress (if set to debug mode) in the same file, but output stack traces to a separate file. Someone else may want to use the Event log. So, I just toss them up if the procedure can't do anything with it.

ekolis

What, you've never played any games by Illwinter Game Design? They love to display that very same message, only in Finnish! "Nagot gick fel", I believe it is... ;)