Don't be a Smart Ass Coder - Exercise in Rafctoring

Here's a piece of code I saw the other day.

the code is for real and given after massive renaming to protect the guilty

for ( int j = 0; j < 2; j ++ )
{
 if ( j == 0 )
   pObject = Repository::GetInstance()
     ->GetObjectByHandle(
       pParent->GetPartAHandle());
 else
   pObject = Repository::GetInstance()
     ->GetObjectByHandle(
       pParent->GetPartBAHandle());

 if ( pObject )
 {
   ObjectGeometry objectGeometry =
     pObject->GetGeometry();

   for ( UINT i = 0;
         i < objectGeometry.NumRectangles();
         i ++)
   {
     Rectangle* pRect =
       Repository::GetInstance()
         ->GetRectanglesByHandle(
           pObject->GetRectnagleHandle(i, 0));
     if ( pRect )
       m_Repository.DiscardRect(
         pRect->GetHandle()
       );
   }
   m_Repository.DiscardSpread(
     pObject->GetHandle());
 }
}

First reaction was WTF !@$^@%@%^

Second reaction was common your not serious

after thing really sunk in, it was "Smart Ass"

Don't be a Smart Ass coder

Really don't! showing how smart you are through code serve no other purpose other then boosting your own ego. the immediate effect however is that it takes just a bit longer for other people to read your code. Yes you probably can deal with that, but show some respect to other people time. why do you make me read such a thing. cant this be made simpler?

of course it can, lets do that:

Step 1

the chunk of the method in fact clear some stuff in pObject, lets state that:

for ( int j = 0; j < 2; j ++ )
{
 if ( j == 0 )
   pObject = Repository::GetInstance()
     ->GetObjectByHandle(
       pParent->GetPartAHandle());
 else
   pObject = Repository::GetInstance()
     ->GetObjectByHandle(
       pParent->GetPartBAHandle());

 CleanObject(pObject);
}

void CleanObject(Object pObject)
{
   if ( pObject )
   {
       ObjectGeometry objectGeometry =
           pObject->GetGeometry();

       for ( UINT i = 0;
           i < objectGeometry.NumRectangles();
           i ++)
       {
           Rectangle* pRect =
               Repository::GetInstance()
               ->GetRectanglesByHandle(
               pObject->GetRectnagleHandle(i, 0));
           if ( pRect )
               m_Repository.DiscardRect(
               pRect->GetHandle()
               );
       }
       m_Repository.DiscardSpread(
           pObject->GetHandle());
   }
}

Step 2

do we really need a for loop with this weird if statement? Not really, in fact using the loop index for branching is probably a code smell.

Also who loop until 2?

use the oldest counting scheme (1, 2, many) and only when reaching many use a for.

so getting rid of that we get to:

pObject = Repository::GetInstance()
 ->GetObjectByHandle(
   pParent->GetPartAHandle());
CleanObject(pObject);

pObject = Repository::GetInstance()
 ->GetObjectByHandle(
   pParent->GetPartBAHandle());
CleanObject(pObject);

Step 3

The resulting CleanObject is doing two things:

1. Clean all the rectangles inside object.
2. Clean up the object itself.

Let separate those:

void CleanObject(Object pObject)
{
   if ( pObject )
   {
       CleanRectnagles(pObject);

       m_Repository.DiscardObject(
           pObject->GetHandle());
   }
}

void CleanRectnagles(Object &pObject)
{
   ObjectGeometry objectGeometry =
       pObject->GetGeometry();

   for ( UINT i = 0;
       i < objectGeometry.NumRectangles();
       i ++)
   {
       Rectangle* pRect =
           Repository::GetInstance()
           ->GetRectanglesByHandle(
           pObject->GetRectnagleHandle(i, 0));
       if ( pRect )
           m_Repository.DiscardRect(
           pRect->GetHandle()
           );
   }
}

Step 4

For good measure lets simplify CleanRectnagles even further:

void CleanRectnagles(Object &pObject)
{
   ObjectGeometry objectGeometry =
       pObject->GetGeometry();

   for ( UINT i = 0;
       i < objectGeometry.NumRectangles();
       i ++)
   {
       CleanRectnaglesExtracted(pObject, i);
   }
}

void CleanRectnagle(Object &pObject, UINT &rectIndex)
{
   Rectangle* pRect =
       Repository::GetInstance()
       ->GetRectanglesByHandle(
       pObject->GetRectnagleHandle(rectIndex, 0));
   if ( pRect )
       m_Repository.DiscardRect(
       pRect->GetHandle()
       );
}

Final Result

and we end up at:

pObject = Repository::GetInstance()
 ->GetObjectByHandle(pParent->GetPartAHandle());
CleanObject(pObject);

pObject = Repository::GetInstance()->
 GetObjectByHandle(pParent->GetPartBAHandle());
CleanObject(pObject);

void CleanObject(Object pObject)
{
   if ( pObject )
   {
       CleanRectnagles(pObject);
       m_Repository.DiscardObject(
           pObject->GetHandle() );
   }
}

void CleanRectnagles(Object &pObject)
{
   ObjectGeometry objectGeometry =
       pObject->GetGeometry();
   for ( UINT i = 0;
       i < objectGeometry.NumRectangles();
       i ++)
   {
       CleanRectnaglesExtracted(pObject, i);
   }
}

void CleanRectnagle(Object &pObject,
                   UINT &rectIndex)
{
   Rectangle* pRect =
       Repository::GetInstance()->
         GetRectanglesByHandle(
           pObject->GetRectnagleHandle(
             rectIndex, 0));
   if ( pRect )
       m_Repository.DiscardRect(
         pRect->GetHandle() );
}

Food For Thought

It still doesn't look clean enough. Looking closely one can easily spot misplaced operations. CleanRectnagles really looks like it belongs to Object class. The same holds true for CleanObject and CleanRectnagle. Was that so obvious in the original code?

CleanRectnagle can use an additional simplification, resulting in a clean method which belongs to the rectangle class.

etc, etc...

Summary

Take a look back at the initial piece of code, yes its a little shorter then the resulting code. However how long would it take you to understand it?

Remember when writing code the goal is not to show how smart you are the goal is to write simple straight forward code which is easily understandable all programmers reading it later.

Posted in: Design,Engineering Practices,Refactoring,tips