Still not sure if this is the worst code we have (but it is a good candidate)



  • (No, i did not mess up the format. It was like this already.)

     

    
    <font size="3">#define MEMCATCH catch(CMemoryException*e) { MemoryError(e); }
    

    void CCode::GetAndInc(char *cod,CString &Code)
    {
    OpenRSet(FALSE,ToModule(cod));

    COleVariant VAL; int N=0,s=0; int pause=Retry.Wait(); repeatReadWrite:
    try
    {
    	CWaitCursor wait;
    	pRS-&gt;GetFieldValue(cod,VAL);
    	
    	char *p=(char*)VAL.bstrVal; N=atoi(p); s=0; while(*p) { s++; p++; }
    	Code.Format("%*d",s,N); N++; CString val; val.Format("%*d",s,N);
    	VAL.SetString(val,VT_BSTRT);
    	
    	pRS-&gt;SetFieldValue(cod,VAL);
    	pRS-&gt;Update();
    	pDoc-&gt;m_lastcodpertable.SetAt(cod,(void*)N);
    }
    MEMCATCH
    catch(CAppException*e)
    {
    	if (Retry.Check(pause,e-&gt;m_strDescription))
    	{
    		e-&gt;Delete();
    		try
    		{
    			pRS-&gt;CancelUpdate();
    			pRS-&gt;Edit();
    			goto repeatReadWrite;
    		}
    		MEMCATCH
    		catch(CAppException *err) { e=err; }
    	}
    	e-&gt;Delete();
    	Close(); AfxThrowUserException();
    }
    pRS-&gt;Close(); delete(pRS); pRS=NULL;
    

    }
    </font>



  • Other than some weird formatting with multiple statements per line (which can be okay in some cases, maybe not this one), and a goto that could probably be eliminated with five minutes of thought, what's the big deal? If this is the worst you've got then you're in good shape.



  • Was this written by a matlab programmer (systems engineer)?

    @smxlong said:

    a goto that could probably be eliminated...what's the big deal?

    A goto that has never been eliminated.


  • Discourse touched me in a no-no place

    @noitcif said:

    @smxlong said:
    a goto that could probably be eliminated...what's the big deal?

    A goto that has never been eliminated.

    Goto has perfectly legitimate uses. Rare, yes, but there are code constructs that require either the use of goto, or lots of involuted if(){}else{} constructs to not use it which are distinctly less clear. It's common in linux kernel drivers for example.



  • @PJH said:

    Goto has perfectly legitimate uses. Rare, yes, but there are code constructs that require either the use of goto, or lots of involuted if(){}else{} constructs to not use it which are distinctly less clear.

    Or, a better language syntax.



  •  Throwing exceptions that are dynamically allocated and assuming the catcher is going to remember to delete it is pretty bad.  That is one of those things that you have to put into a developer's manual and hope everyone reads it.

     e->Delete () gets called twice under certain circumstances.  You have to assume knowledge of the internals of that object in order to know if calling the Delete() method twice works or causes a double memory deallocation.

     Using a macro to define an exception handler is lame.  That particular goto is asking for trouble.

    Yeah, I'd say this is pretty bad.


  • Considered Harmful

    Which circumstances?

    e gets deleted, and either a goto occurs or an exception gets thrown and e gets reassigned to the new exception pointer, which then gets deleted as well as the first.

    It may be stupid but it doesn't seem broken.



  •  @joe.edwards said:

    Which circumstances?

    e gets deleted, and either a goto occurs or an exception gets thrown and e gets reassigned to the new exception pointer, which then gets deleted as well as the first.

    It may be stupid but it doesn't seem broken.

     

    e->Delete () is called in the scope of <font><font size="3">CAppException*e both times.  If a CMemoryException is thrown, it is a different variable e operated on in the scope of that macro.  It is hard to tell if MemoryError(e) returns or re-throws the exception.  Assuming it returns, the second e->Delete is called on the e from the original scope that was already deleted.</font></font>

     

    Broken?  Probably not, at least not totally.  This type of code causes those instances where "we have this really weird problem where the program crashes every so often for no apparent reason and we can't recreate it".  This is code that doesn't expect the unexpected to happen.

     


  • Considered Harmful

    I'd assumed MEMCATCH was some kind of no-op, a preprocessor definition that expands to nothing.



  • Did you read the code?



  • Let's count the WTFs:

    1. Awful formatting,
    2. Using a macro that expands to a catch block,
    3. Throwing an exception for an operation that should be retried instead of retrying the operation,
    4. Testing an (assumed) string member of the exception to determine whether to retry,
    5. Using goto to implement a while loop,
    6. Deleting the exception by calling a member function instead of using the delete operator,
    7. Calling a function (apparently) dedicated to throwing an exception, and finally
    8. Throwing pointers to objects in C++.

    Protip: throw temporaries and catch const references unless you have a good reason to be different. Your life will suck less.



  • @ooblek said:

    e->Delete () is called in the scope of <font><font size="3">CAppException*e both times.  If a CMemoryException is thrown, it is a different variable e operated on in the scope of that macro.  It is hard to tell if MemoryError(e) returns or re-throws the exception.  Assuming it returns, the second e->Delete is called on the e from the original scope that was already deleted.</font></font>

     

     

    MemoryError might also cry "out of memory" and quit the program, which might be the most sensible thing to do when memory allocation fails.



  • @ammoQ said:

    MemoryError might also cry "out of memory" and quit the program, which might be the most sensible thing to do when memory allocation fails.

    I can just see it now: throw new MemoryError("We're screwed!");



  • I can't beleive it's this late in the piece and nobody has said "TRWTF is MFC". Not to mention a char* and a CString& in the same function signature. And before anyone brings up some copy-by-value optimization justification for this, consider how many other languages have 3+ ways of representing a string. std::string, CString, char*, then there's BString... etc, and i'm sure OLE probably gets it's own string class too.


Log in to reply