Fog Creek Software
Discussion Board




Please code review this Java snippet

Here's a snippet of some Java code I've come across recently. Your opinions and comments would be welcome. I'll post some background info later, for now I don't want to influence comments one way or the other. Thanks in advance.


////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
// Class Name : {foo}DataEventHandler
// This class extends the EventHandler Class and implements the handle ( Map )                                                 
///
// method.                         
///
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////


    /* +-----[CommentLine1]  */
    /* | */
    /* | */ if ( myProxy.getExportCommentLine1 ()  != null )
    /* | */    dataOut.put ( "CommentLine1" , myProxy.getExportCommentLine1 ()  );
    /* | */
    /* +-----[CommentLine2]  */
    /* | */
    /* | */  if ( myProxy.getExportCommentLine2 ()  != null )
    /* | */    dataOut.put ( "CommentLine2" , myProxy.getExportCommentLine2 ()  );
    /* | */
    /* +-----[DateCreated]  */
    /* | */
    /* | */  try
    /* | */  {
    /* | */    if ( myProxy.getExportDateCreated ()  != null )
    /* | */      dataOut.put ( "DateCreated" ,  MyAdapterHelper.calendarToDateString ( (Calendar) myProxy.getExportDateCreated ()  )  );
    /* | */  }
    /* | */  catch ( Exception e )
    /* | */  {
    /* | */    handleExportError ( "Error while Exporting field DateCreated[Value= " + myProxy.getExportDateCreated ()  + "]", e );
    /* | */  }
    /* | */
    /* +-----[DateReleased]  */
    /* | */
    /* | */  try
    /* | */  {
    /* | */    if ( myProxy.getExportDateReleased ()  != null )
    /* | */      dataOut.put ( "DateReleased" ,  MyAdapterHelper.calendarToDateString ( (Calendar) myProxy.getExportDateReleased ()  )  );
    /* | */  }
    /* | */  catch ( Exception e )
    /* | */  {
    /* | */    handleExportError ( "Error while Exporting field DateReleased[Value= " + myProxy.getExportDateReleased ()  + "]", e );
    /* | */  }
    /* | */
    /* +-----[TimeCreated]  */
    /* | */
    /* | */  try
    /* | */  {
    /* | */    if ( myProxy.getExportTimeCreated ()  != null )
    /* | */      dataOut.put ( "TimeCreated" ,  MyAdapterHelper.calendarToTimeString ( (Calendar) myProxy.getExportTimeCreated ()  )  );
    /* | */  }
    /* | */  catch ( Exception e )
    /* | */  {
    /* | */    handleExportError ( "Error while Exporting field TimeCreated[Value= " + myProxy.getExportTimeCreated ()  + "]", e );
    /* | */  }
    /* | */
    /* +-----[UseridCreated]  */
    /* | */
    /* | */  if ( myProxy.getExportUseridCreated ()  != null )
    /* | */    dataOut.put ( "UseridCreated" , myProxy.getExportUseridCreated ()  );
    /* | */
    /* +-----[UseridReleased]  */
    /* | */
    /* | */  if ( myProxy.getExportUseridReleased ()  != null )
    /* | */    dataOut.put ( "UseridReleased" , myProxy.getExportUseridReleased ()  );
    /* | */
    /* +-----[Code]  */
    /* | */
    /* | */  if ( myProxy.getExportCode ()  != null )
    /* | */    dataOut.put ( "Code" , myProxy.getExportCode ()  );
    /* | */
    /* +-----[Description]  */
    /* | */
    /* | */  if ( myProxy.getExportDescription ()  != null )
    /* | */    dataOut.put ( "Description" , myProxy.getExportDescription ()  );
    /* | */
  }
  catch ( Exception e )
  {
    throw new Exception (  e.getMessage () );
  }

anon
Monday, July 28, 2003

Apart from all the noise of comments down the left hand side there's an egregious amount of double calling going on, test output of function, if output call some other function with the output of the function again; repeating in a catch phrase the function that could be causing the try to fail is also interesting.

Its hard to tell whether the actual data stream means that all of those conditions or some subset could be true all at the same time and the same invocation or whether they are all possible but singleton events at any one time.

If there is only one possible event at any one time then it would be better as a switch and better to call the particular function just once.  In a different language I'd probably have the calling functions in some kind of table the 'Datahandler' would then be a generic read/call branch part of some kind of finite automata no doubt.

Simon Lucy
Monday, July 28, 2003

If I can modify/subclass the code of 'dataOut', I'd make it do nothing if the 2nd parameter is null, since that's basically what all the 'if' statements are doing.

rexguo
Monday, July 28, 2003

WTF?

I hope that's autogenerated from some kind of literate programming tool. Otherwise, whoever wrote it has far too much time on their hands. I'd argue all the comments render it unreadable.

I note also that most of the cases are similar -- it should be possibly to use a loop to perform them all. I wouldn't like to give any extra details because this kind of thing often involves a bit of tweaking somewhere, and my Java experience is a bit limited (you could use the introspection facilities I'm sure?) -- but I've often found it advantageous to make this kind of change in my own code.

Tom
Monday, July 28, 2003

It looks like you have a whole bunch of different data objects, each of which has two operations: getting and putting.  It seems natural to me to have a class for each data object, and have them all implement the same interface.

Alyosha`
Monday, July 28, 2003

The Catch block seems pretty pointless...

John Topley (www.johntopley.com)
Monday, July 28, 2003

Yeah, what is the point of the catch block? Its only purpose that I can see is to a) give you a place to put a break point when you are debugging b) ensure that only Exceptions and not any descendants of Exception are thrown. Is there another reason?

David Clayworth
Monday, July 28, 2003

It's Java -- it will catch any Exception-derived exception.

dataOut.put might throw something -- I guess that's why it's there.

Tom
Monday, July 28, 2003

Sure, but the code consumes an Exception, then throws another Exception...  Why do that when the original exception could be thrown?

Joe V
Monday, July 28, 2003

Catching exceptions so that you can do some error handling before forwarding it to another handler higher up in the call stack.

rexguo
Monday, July 28, 2003

There must be a school or a book that teaches this kind of stuff.

It looks a lot like a lot (far too much) of code I've had to work with [shudder].

Walter Rumsby
Monday, July 28, 2003

This is nasty. Absolutely nasty.

Whoever wrote that is an idiot.

1) the comments are asinine. The author clearly has no idea how comments should be used. In any language. Based on the commenting style, I estimate that the author is 12 years of age.

2) Do not catch java.lang.Exception - catch the specific type of exception

3) the last try-catch is one of the most stupid things I have seen.

4) dataOut.put method should handle nulls. An overloaded version of this method should handle the calendar stuff.

... and so on.

Yep, it's bad. Extrordinarily bad.

burninator
Tuesday, July 29, 2003

Thanks for the responses everybody. This is some code my team is stuck supporting. I didn't give enough information to tell, but the biggest bug here is that if the value is null, it still needs to get passed along in the output stream. The guy on my team who is stuck with this looked at it and said basically what burninator said. I agreed and floated it up the chain that the adaptor code had some serious flaws and was written in style that screams "the lights are on but nobody's home."

The bigger problem is that it's written by the new flavor-of-the-month, super expensive, 10 digit annual revenues consulting company. I figured if I slammed it they'd just get some clever charlatan to slam me by pointing out how all these flaws are evidence of what a great job they do. Then the non-technical managers are in the unenviable position of arbitrating a tech dispute. So... I thought I throw it to an independent panel of experts - naturally I thought of you guys. Thanks again.

Jim S.
Tuesday, July 29, 2003

*  Recent Topics

*  Fog Creek Home