Refactoring game legacy code Part 2 – Cleaning up the terrain

29 07 2009

In last episode, I introduced my project about refactoring my Tetris clone code.

In order to get a feel of the code, I’m going to do some small mechanical refactorings. I will do only safe renames and extract method in the class related to Block and Piece. I will not try to alter behavior before I write my characterization tests (will explain in a later article). I am using Visual Studio 2005 and Code Rush(which include Refactor! C++) free version. Visual Assist X will be fine too but it costs money.

Let’s go deep into the code !

The first thing I noticed when reading the code related to Piece and Block was this:

void Piece::moveBlocks(int deltaX, int deltaY)
{
 for(int i=0; i<TetraSize; i++)
 {
  m_tetra&#91;i&#93;.move( m_tetra&#91;i&#93;.x() + deltaX, m_tetra&#91;i&#93;.y() + deltaY );
 }
}
&#91;/sourcecode&#93;

Why do I need to query the X and Y coordinates of each block forming the tetra in order to move them ? This is a violation of the principle "<a title="Tell, don't ask" href="http://www.pragprog.com/articles/tell-dont-ask">Tell, don't ask</a>". I'm going to refactor in small steps (like strongly suggested in Martin Folwer's Refactoring).

First I renamed the <code>move</code> method of Block class to <code>moveAbsolute</code> since it's clearly what this method is doing. After, I created a new method called <code>Block::move</code> which really moved according to deltaX and deltaY. Then, I looked up all the calls of moveAbsolute and make then use the new <code>move()</code> if they required this new behavior.

I found a interesting thing. All calls to <code>moveAbsolute</code> are in my poor handling of the Piece. Thus in the future, we will be able to remove this method after several refactorings. This single rename make my bad code stood out of the rest.

After, my random look into the code, I saw this big function:


SDL_Surface *BlockSurfaceManager::get(BlockColor color)
{
 if( d_ptr->surfaceCache.count(color) == 0 )
 {
  SDL_Surface *newSurface = SDL_CreateRGBSurface(SDL_HWSURFACE, BlockWidth, BlockHeight, ColorDepth, 0, 0, 0, 0);

  SDL_Rect blockRect;
  blockRect.x=0;
  blockRect.y=0;
  blockRect.w=BlockWidth-1;
  blockRect.h=BlockHeight-1;

  Uint8 r=0,g=0,b=0;
  switch(color)
  {
   case BlockCyan:
    r=0; g=240; b=240;
    break;
   case BlockBlue:
    r=0; g=0; b=240;
    break;
   case BlockOrange:
    r=240; g=160; b=0;
    break;
   case BlockYellow:
    r=240; g=240; b=0;
    break;
   case BlockGreen:
    r=0; g=240; b=0;
    break;
   case BlockPurple:
    r=160; g=0; b=240;
    break;
   case BlockRed:
    r=240; g=0; b=0;
    break;
  }

  SDL_FillRect(newSurface, &blockRect, SDL_MapRGB(newSurface->format, r,g,b));

  d_ptr->surfaceCache[color] = newSurface;
 }

 return d_ptr->surfaceCache[color];
}

As you can see, the if block is creating a new surface if the color has not been asked yet. The problem is this block is in the way of the main logic of my method, which is to retrieve a cached surface for the given color. Using “Extract Method”, I can make my intent more clear.

SDL_Surface *BlockSurfaceManager::get(BlockColor color)
{
 if( d_ptr->surfaceCache.count(color) == 0 )
 {
  createCachedSurfaceFor(color);
 }

 return d_ptr->surfaceCache[color];
}

I could go very zealot while refactoring this class. For the intend of this article, I’m going to ‘give my life for Aiur’ errrr make my intention even more clear 🙂

I will extract the if condition into its own method:

SDL_Surface *BlockSurfaceManager::get(BlockColor color)
{
 if( hasNoCachedSurfaceFor(color) )
 {
  createCachedSurfaceFor(color);
 }

 return d_ptr->surfaceCache[color];
}

bool BlockSurfaceManager::hasNoCachedSurfaceFor(BlockColor &color)
{
 return d_ptr->surfaceCache.count(color) == 0;
}

As you can see, it makes my intent more clear. Also, when you extract method with CodeRush, please double check the header if your method has the correct visibility.

For the beverity of the article, I will not describe in details the mechanics of the refactoring of createCachedSurfaceFor() but here’s for your viewing pleasure:

void BlockSurfaceManager::createCachedSurfaceFor(BlockColor &color)
{
 d_ptr->surfaceCache[color] = createBlockSurface(color);
}

SDL_Surface* BlockSurfaceManager::createBlockSurface(BlockColor &color)
{
 SDL_Surface *newSurface = SDL_CreateRGBSurface(SDL_HWSURFACE, BlockWidth, BlockHeight, ColorDepth, 0, 0, 0, 0);

 Uint8 r=0,g=0,b=0;

 getBlockColorRgb(color, r, g, b);

 drawBlockSurface(newSurface, r, g, b);

 return newSurface;
}

void BlockSurfaceManager::drawBlockSurface(SDL_Surface *newSurface, Uint8 &r, Uint8 &g, Uint8 &b)
{
 SDL_Rect blockRect = createBlockRect();
 SDL_FillRect(newSurface, &blockRect, SDL_MapRGB(newSurface->format, r,g,b));
}

SDL_Rect BlockSurfaceManager::createBlockRect()
{
 SDL_Rect blockRect;
 blockRect.x=0;
 blockRect.y=0;
 blockRect.w=BlockWidth-1;
 blockRect.h=BlockHeight-1;
 return blockRect;
}

void BlockSurfaceManager::getBlockColorRgb(BlockColor &color, Uint8 &r, Uint8 &g, Uint8 &b)
{
 switch(color)
 {
 case BlockCyan:
  r=0; g=240; b=240;
  break;
 case BlockBlue:
  r=0; g=0; b=240;
  break;
 case BlockOrange:
  r=240; g=160; b=0;
  break;
 case BlockYellow:
  r=240; g=240; b=0;
  break;
 case BlockGreen:
  r=0; g=240; b=0;
  break;
 case BlockPurple:
  r=160; g=0; b=240;
  break;
 case BlockRed:
  r=240; g=0; b=0;
  break;
 }
}

The code has more lines, but the intend of the code is more clear. I stop here for the moment but I should really factor the surface creator into its own class. BlockSurfaceManager has two responsibilities: Managing the surface cache AND creating the surface. This does not follow the Single Responsibility Principle where each class should have one reason to change.

I will commit the changes I’ve made. Let’s return back to Piece. In Piece::rotate() I have this snipped of code

if( type() == Piece_Square )
 {
  m_direction = newDirection;
  return;
 }

 Block *rotatedBlocks = getBlocks(newDirection);
 if( tryMove(0, 0, rotatedBlocks) )
 {
  delete[] m_tetra;
  m_tetra = rotatedBlocks;
  m_direction = newDirection;
 }
 else
 {
  delete[] rotatedBlocks;
 }

I smell something bad in here. The base class make assumption about its derived classes. I will leave it here for now because I haven’t written my test and I could alter behavior by removing the faulty block.

Here’s another obscure method:

bool Piece::tryMove(int deltaX, int deltaY, Block *block)
{
 bool canMove=true;

 for(int i=0; i<TetraSize; i++)
 {
  if( (block&#91;i&#93;.x() + deltaX) >= BoardEndX ||
   (block[i].x() + deltaX) < BoardX ||
   m_board->blockAt(block[i].x() + deltaX, block[i].y() + deltaY) ||
   (block[i].y() + deltaY) < BoardY ||
   (block&#91;i&#93;.y() + deltaY) >= BoardEndY
   )
  {
   canMove=false;
   break;
  }
 }

 return canMove;
}

The if condition is confusing as hell. With simple reordering and extract method, it becomes this:

if( isOutsideBoard(block[i], deltaX, deltaY) ||
   hasBlockingBlock(block[i], deltaX, deltaY) )
  {
   canMove=false;
   break;
  }
bool Piece::isOutsideBoard(const Block &block, int deltaX, int deltaY) const
{
 return (block.x() + deltaX) >= BoardEndX ||
   (block.x() + deltaX) < BoardX ||
   (block.y() + deltaY) < BoardY ||
   (block.y() + deltaY) >= BoardEndY;
}

bool Piece::hasBlockingBlock(const Block &block, int deltaX, int deltaY)
{
 return m_board->blockAt(block.x() + deltaX, block.y() + deltaY);
}

Make much more sense. Let’s commit this. As for the derived classes of Piece, I won’t ever dare to touch them before I write some tests.

Let’s call done for the day. In the next episode, I will show you how to make the code into test harness.

Advertisements

Actions

Information

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s




%d bloggers like this: