Whats Wrong With This Code

Here's an actual example from the interview guidelines of a company I once interviewed with. The game is "What's wrong with this code?"

 int A( CBar *pBar )
 {
CFoo *pFoo;

pFoo = new CFoo( pBar );
if( NULL == pFoo )
return( -1 );

if( !pBar->Validate() )
delete pFoo;

return( pFoo->GetValue?() );
 }

 // where there is a constructor for Cfoo:
 CFoo::CFoo( CBar * );

I was told the given correct answer was that (to summarize) the method should be written:

 int A( CBar *pBar )
 {
CFoo *pFoo;

pFoo = new CFoo( pBar );
if( NULL == pFoo )
return( -1 );

if( !pBar->Validate() )
{
delete pFoo;
return -1;
}

return( pFoo->GetValue?() );
 }

But do you see what else is wrong? Ordered from most innocuous to most ludicrous:

• Parentheses around return value. I never understood this, but it's an affectation of programmers who have only worked in C derived languages.
• Use of non-standard macro, NULL. C++ now uses 0.
• CFoo *pFoo should be initialized where it is declared.
• Likely failure of ConstCorrectness. CBar *pBar should probably be CBar const *pBar;. While it's unclear whether this is true, given the other code examples in the interview, the target company doesn't know what const is. (*)
• pFoo still leaks! I can attest the "C++" programmers there are actually Java programmers, and thus they actually, really don't understand what new and delete actually do, let alone the stack. The easy fix is to make CFoo *pFoo = new CFoo( pBar ); into CFoo foo( pBar ); Yes, one can logically conclude that if one is a Java programmer, that they do not understand new, delete, and the stack Sarcasm? If they are Java programmers, and have never properly learnt C++, then it is logically correct to conclude that, as new, delete, and the C++ stack operate very differently than Java's GarbageCollector.
• Global function instead of (class) method. Mixed paradigm code, especially like the above whose internals are all OO, is a CodeSmell.
• Stupid use of object-oriented programming. Minimally, this probably is a result of a violation the LawOfDemeter inside of CFoo. CBar should return the value directly.
• if pBar->Validate() returns false, the function deletes pFoo then promptly dereferences it. UndefinedBehavior is bad.

Other criticisms?

• In a conforming implementation, new should never return 0 - if the required memory is not available, it should throw. [This is true with systems that enable exception handling. The company works in the embedded space. I apologize. I neglected to qualify that. -- ss]
• If you insist on allocating the CFoo on the heap, why not use a std::auto_ptr<>? [Templates don't exist. Don't believe the hype! The STL is doubly non-existent. At least on embedded systems, where it seems compilers are thrown together at lunch time. -- ss]
• If !pBar->Validate(), you don't even need a CFoo object anyway [I thought this, too. Then I realized there might be side effects in the constructor. This is bad style but so is most of that method. -- GrahamHughes]
• Should have a ASSERT ( pBar!=0 ); [or assert( pBar )]

(*) Further, we have the following question

Write StrCpy?() where the prototype is,

uint strcpy( char *pDst, char *pSrc );

which is bogus on many levels. Ignoring the BadVariableNames, the correct signature is

char *strcpy( char *pDestination, char const *pSource );

But later on, the question appears

What does "const" mean?

which leads me to believe the programmers at this company don't know enough C++ to conduct the interview, let alone do their jobs. Actually, to be fair, the interview has some good questions relative to the rest of the interviews with code I've seen, but that only depresses me further. -- SunirShah

---

"Ignoring the BadVariableNames" how can you do that?

My first reply would be "these variable suck". To quote MartinFowler: "Any fool can write code that a computer can understand, Good programmers write code that humans can understand. "

MetasyntacticVariables are always given as examples in computer books, but I think it's a dangerous practice. It implies tacit validation that this code is 'proper', after all, some hot-shot programmer got paid to write the book.

---

What does "const" mean?

Hmmm. Would you consider Discuss the pros and cons of using const a better question? Although, it may wind up being a filter, selecting those who have read ScottMeyers from those who haven't...