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:
- Clean all the rectangles inside object.
- 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.
4/28/2010
Posted in: Random Thoughts
some visual studio add-ins and plug-ins. I'm testing a few new ones and even playing around learning how to develop one of my own. Doing so did cause me to break the IDE leaving me in all sorts of weird situations. For example one time I messed it so bad I couldn't even create a new .NET project any more. For some reason the IDE insisted that I'll install some language tools (or something alike).
Posts
