Coding challenge, crowd-sourcing finding the answers

So, I mentioned the Blackhat coding challenge the other day.  I still haven’t seen the answers out there pop up in the wild, wild, world of web, and it’s got me frustrated.  So in the interests of figuring out if I’m right or not, I figure I’ll post my thoughts here and “crowd source” finding out if I’m right or not.

Challenge #1

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

In the unlikely event that I’m right, putting this here could be a bit spoilerey (I don’t want to step on anyone’s good time), so please stop reading RIGHT NOW in the case you don’t want to see a possible answer (no guarantee btw that it is the right one).

My answers:

So there are a few issues I think with this – some stylistic problems and “the whammy”.  The small stuff is:

• Use of second const is unnecessary
• Passing by reference (e.g. const someClass & newObj) instead of the pointer could be safer here since the compiler will help enforce validity
• newObject is not tested for validity before being passed to the equals() method
• The return statement is complicated and hard to read.  Recommend breaking it out to multiple lines
• Return value tests for null pointer but does not test for validity (if non-null) – a function/macro like ISVALIDPTR/IsValidPtr() would help this situation when passing pointer instead of reference
• Return value indicates both success/fail as well as result.  Recommend structured exception handling and returning only the result

Stylistic issues… blah.  But then there’s this:

Big problem: The return statement resolves right to left, meaning you’re asking the compiler to dereference oldObject before you’ve tested it for validity.  Best case, you crap out.  Worst case, it’s a security problem.

Challenge #2

wchar_t *fillString(
    wchar_t content, unsigned int repeat)
{
    wchar_t *buffer;
    size_t size;
    if (repeat > 0x7fffffffe)
        return 0;
    size = ( repeat + 1 ) * sizeof content;
    buffer = (wchar_t *) malloc ( size );
    if ( buffer == 0 )
        return 0;
    wmemset(buffer, content, repeat);
    buffer[ repeat ] = 0;
    return buffer;
}

Again, please not to keep reading if you don’t want to hear my thoughts on the solution.

Small stuff:

• buffer and size are not initialized
• if this is C++, why malloc instead of new?
• Comparison with repeat should be a #define (e.g. if repeat > MAX_SIZE) instead of an architecture/OS dependent value, remembering to take into account that wchar_t is of variable size (usually either 16 or 32 bit)
• Recommend return comparison on malloc result be NULL instead of 0 (usually NULL == 0, but I find it more readable)
• Recommend structured exception handling rather than returning null pointer if there is an issue (e.g. if (!buffer) { throw MemoryException(“Memory could not be allocated”); } or some such

Again, little stuff is stylistic.  Bigger problem IMHO is this:

Big problem: it’s a wide character buffer.  And 0 is not the literal for a multibyte null.  You’re gambling on the compiler doing the right thing, which it may or may not.   This may work, but it’s a crap-shoot.  Replace with L’\0’, the multibyte character literal null (if one were going to fill the string this way – which I also wouldn’t recommend that you do.)

So that’s what I came up with.  Anybody else find anything different?