Amazon Black Hat coding question

I’m at Black Hat, hanging out at the Amazon booth with my colleagues, and quite amazed at the stir created by the coding question I put up on the board.

Vegas 017

For those of you that haven’t had a chance to see the code, or need more time to figure out what’s wrong, I’m posting the code here. Sadly, no prizes for those of you who aren’t able to come to the booth, and I won’t be accepting posts commenting on the code until after the conference is over.Vegas 015

Consider it a theoretical exercise in code review – how would you handle this Java code arriving in front of you at a code review?

public static boolean isDifferent(
    final MyBean oldBean,
   
final MyBean newBean)
{
      return
        oldBean != newBean &&
       
oldBean != null &&
       
!oldBean.equals(newBean);
}

Post your comments, and after the conference is over, I’ll open them up.

For those of you that think in C++ instead of Java (and there were many at the conference), here’s a version in that language:

public:
    static bool isDifferent(
        const someClass const * oldObject, 
        const someClass const * newObject)
    {
        return
            oldObject != newObject &&
            oldObject != NULL &&
            !oldObject->equals(newObject);
    }


There are no ‘cheat’ issues – code not present in the sample is assumed to be perfect.



.csharpcode, .csharpcode pre
{
font-size: small;
color: black;
font-family: consolas, “Courier New”, courier, monospace;
background-color: #ffffff;
/*white-space: pre;*/
}
.csharpcode pre { margin: 0em; }
.csharpcode .rem { color: #008000; }
.csharpcode .kwrd { color: #0000ff; }
.csharpcode .str { color: #006080; }
.csharpcode .op { color: #0000c0; }
.csharpcode .preproc { color: #cc6633; }
.csharpcode .asp { background-color: #ffff00; }
.csharpcode .html { color: #800000; }
.csharpcode .attr { color: #ff0000; }
.csharpcode .alt
{
background-color: #f4f4f4;
width: 100%;
margin: 0em;
}
.csharpcode .lnum { color: #606060; }

8 Responses to Amazon Black Hat coding question

  • Jürgen Menden says:

    I see only semantic problems.

    1. old->equals probably means old->isEqual, if not it might try to make newObject equal to old or the other way round. Which btw. fails because of the ‘final’

    No, .equals is defined at http://www.leepoint.net/notes-java/data/expressions/22compareobjects.html (and other places) as a comparison operator, designed not to change either value.

    2. isDifferent(obj, obj) with the same object on both parameters will give False, which is as well very counter-intuitive.

    That’s certainly a bad thing, and would be worth bringing up in code review – perhaps even to the extent of rejecting the function’s name and use. But it’s not the actual flaw.
    Tuesday, August 31, 2010 16:10 PM by Alun Jones
  • pilcrow says:

    isDifferent(some_obj, null) -> true
    isDifferent(null, some_obj) -> false

  • Tom says:

    It returns false if oldBean and newBean are the same object.

    It should eliminate that first expression from the return statement (oldBean != newBean). equals() is sufficient.

    OK, no.

    isDifferent(a,a) should return false, so your first sentence is incorrect. Your second sentence doesn’t fix the function either, and may actually introduce more errors.

    Tuesday August 31, 2010 16:14 PM by Alun Jones
  • interesed says:

    Care to explain what is wrong with the code?

  • Artiom Gourevitch says:

    isDifferent(null, newBean) will return false because oldBean != null is false

  • Mandar J says:

    Fails for
    isDiff ( null, obj )
    expected: true
    actual: false

    How abt the following?
    oldBean != newBean &&
     ( oldBean == null
               ||
               newBean == null
               ||
            ! oldBean.equals(newBean) ); 

  • P A says:

    if oldBean is null method always returns false.

  • bernd says:

    Is there an official correct answer?

    re: Official answers…

     

    I’m not quite ready to post anything that I would call “official answers” yet – but you should be able to see some answers above that you can demonstrate to yourself as correct.

    Perhaps the best answer, however, is that code like this should have been rejected without even evaluating if it is correct in function, because it is so hard for average developers to read.

     


    Tuesday, December 14, 2010 6:55 AM by Alun Jones

Leave a Reply

Your email address will not be published. Required fields are marked *


*

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>