Code review from hell



  • <meta http-equiv="CONTENT-TYPE" content="text/html; charset=utf-8"><title></title><meta name="GENERATOR" content="OpenOffice.org 3.0 (Win32)"><style type="text/css">
    </style>
    

    I've been contracted by big IT player to assist in the review of custom code for one of their clients where that code was developed by another major IT consulting company. While WTFs abound in this code, many are hard to succinctly describe. This little gem, however, I found to be compact and just WTF worthy enough to share. I invite others to examine this code for themselves and comment on how many WTFs they can find.

    Here is some background info to provide context for the code:

    1. It's a J2EE application

    2. ExtendedObjectBookWrapperBean and ObjectWrapperBean are wrappers for 1.x spec entity EJBs.

    3. Some of the object and method names have been changed to maintain confidentiality of various parties involved.

    4. The code has been reformatted with comments largely removed except for one which I found pertinent to the WTF quality of the code.

    5. Exception handling condensed for readability.

    Without further ado, here's the code...

    public class SomeLogicClassImpl {
    // ...
    private String getLatestObjectID() throws WrapperException {
    final String METHOD_NAME = "SomeLogicClassImpl.getLatestObjectID()";
    logger.entering(CLASS_NAME, METHOD_NAME);
    ExtendedObjectBookWrapperBean objExtendedObjectBookWrapperBean = null;
    ArrayList arrListLatestEdited = null;
    ObjectWrapperBean objectWrapperBean = null;
    String strObjectID = null;
    try {
    // Get the last inserted (latest) Object ID of the user in
    // Object Table.
    objExtendedObjectBookWrapperBean =
    new ExtendedObjectBookWrapperBean();
    logger.finest("User ID in getLatestObjectID() = "
    + getUserId());
    objExtendedObjectBookWrapperBean.setUserId(getUserId());
    WrapperBeanManager.populate(objExtendedObjectBookWrapperBean);
    if (objExtendedObjectBookWrapperBean != null) {
    arrListLatestEdited = objExtendedObjectBookWrapperBean
    .getLatestEditedObjects();
    if (arrListLatestEdited != null) {
    logger.finest("Size of arrListLatestEdited = " +
    arrListLatestEdited.size());
    if (arrListLatestEdited.size() > 0) {
    objectWrapperBean = (ObjectWrapperBean)
    arrListLatestEdited.get(0);
    strObjectID = objectWrapperBean.getObjectId();
    logger.finest("Latest Object for the user = "
    + getUserId() + " is : " + strObjectID);
    }}}}
    catch (Exception exception) {
    logger.logp(Level.SEVERE, CLASS_NAME, METHOD_NAME,
    "Error performing " + CLASS_NAME, exception);
    throw new WrapperException(CLASS_NAME, METHOD_NAME, exception, true);
    }
    logger.exiting(CLASS_NAME, METHOD_NAME);
    return(strObjectID);
    }
    // ...
    }

    public class ExtendedObjectBookWrapperBean extends ObjectBookWrapperBean {
    // ..
    public ArrayList getLatestEditedObjects() {
    ArrayList objectList = new ArrayList();
    String userId = getUserId();
    int length=0;
    ObjectWrapperBean objectBean= new ObjectWrapperBean();
    try {
    Enumeration orderedObjectsEnum = objectBean.findByUserIdAndOrderByLastUpdate(userId);
    length = getEnumerationSize(orderedObjectsEnum);
    objectList = new ArrayList(length);
    orderedObjectsEnum = objectBean.findByUserIdAndOrderByLastUpdate(userId);
    while (orderedObjectsEnum.hasMoreElements()) {
    objectList.add((ObjectWrapperBean)orderedObjectsEnum.nextElement());
    }}
    catch (Exception e) {
    logger.info("Exception occured"+e.toString());
    }
    return(objectList);
    }
    public static int getEnumerationSize(final Enumeration enum) {
    int length=0;
    while (enum.hasMoreElements()) {
    length++;
    enum.nextElement();
    }
    return(length);
    }
    // ..
    }

    FYI: At the point where SomeLogicClassImpl.getLatestObjectId() was
    being

    called the calling logic had just added a record in the "Object" table

    by calling some other logic which could have easily been modified to

    return the key value for the newly created record, but instead the

    developer thought this was the best way to go about getting the key

    value for that newly created record.




  • @DeepThought said:

    1. Some of the object and method
      names have been changed to maintain confidentiality of various
      parties involved.


    2. The code has been reformatted with
      comments largely removed except for one which I found pertinent to
      the WTF quality of the code.


    3. Exception handling condensed for
      readability.

    Assuming that you didn't butcher the code worse in the process...

    @DeepThought said:

    FYI: At the point where SomeLogicClassImpl.getLatestObjectId() was
    being

    called the calling logic had just added a record in the "Object" table

    by calling some other logic which could have easily been modified to

    return the key value for the newly created record, but instead the

    developer thought this was the best way to go about getting the key

    value for that newly created record.

    Err, ok.  So what you're saying is there's about 70 counts of WTF besides those obvious in the code above.  That's always helpful to increase the WTFs per line count.

    @DeepThought said:

    		}}}}

    OMG, it's like someone sought out the worst code atrocity I've committed which can be ported to Java, simply to include it in this code.

    At any rate, it's late, so I think I'll quit now.  I found about 72 WTFs, which is close to one per line.  That is, at least, a respectable showing, although if I'd read this earlier in the day, I expect I could've found more.  I'm calling it a night.



  • <meta http-equiv="CONTENT-TYPE" content="text/html; charset=utf-8"><title></title><meta name="GENERATOR" content="OpenOffice.org 3.0 (Win32)"><style type="text/css">
    </style>
    

    @tgape said:

    ...

    Assuming that you didn't butcher the code worse in the process...

    We aren't making any changes to the code, only reviewing it. We will, however, be presenting a long list of changes we'd like to see get implemented. Ripping out much of this code will be only one of the more minor changes we'd like to see implemented.

    @tgape said:

    Err, ok. So what you're saying is there's about 70 counts of WTF besides those obvious in the code above. That's always helpful to increase the WTFs per line count.

    That sums it up pretty much. Now imagine millions of lines of this crap and you can see why I referred to this as the code review from hell.

    @tgape said:

    @DeepThought said:

    }}}}

    OMG, it's like someone sought out the worst code atrocity I've committed which can be ported to Java, simply to include it in this code.

    The condensing of the curly brackets was my doing and it was only so keep the number of lines in the forum post to a minimum. I certainly don't code that way. Of course, the original code was far less consistent with regard to it's formatting.

    Thanks!



  • @DeepThought said:

    It's a J2EE application

    You could have stopped there.



  • @firecow said:

    @DeepThought said:

    It's a J2EE application

    You could have stopped there.

     

    My mother was a J2EE application, you insensitive clod!



  • @Someone You Know said:

    @firecow said:

    @DeepThought said:

    It's a J2EE application

    You could have stopped there.
     

    My mother was a J2EE application, you insensitive clod!

    And your father smelt of Bing©leberries



  • @Someone You Know said:

    My mother was a J2EE application, you insensitive clod!

    No.

     

    Slashdot memes are just as bad as xkcd and are forbidden by the contract you signed when you joined.



  • @morbiuswilters said:

    Slashdot memes are just as bad as xkcd and are forbidden by the contract you signed when you joined.

     

    IMHO, slashdot references are infinitely worst than XKCD references.


  • ♿ (Parody)

    @tster said:

    IMHO, slashdot references are infinitely worst than XKCD references.
    In TDWTF, morbius slashdots XKCD!


Log in to reply