Dusk and Dawn

Conventional wisdom and a lot of research on programming practices generally agree that functions that are short enough not to need to be scrolled are easier to read and less prone to error. Of course, in practice, it’s more often than not that the typical programmer finds themselves writing functions often several pages in length.

At times like those, it’s crucial to take the time out to reorganize and refactor such functions. The long-term benefits in code organization and readability are almost always worth it.

So, dear programmer, when you’re looking over your most recent changes and you find yourself having to scroll down (or, even worse, right!) more than once, take some time out and consider: How can I organize these functions to make them easier to understand? Is this function doing more than it was originally designed to do? Can I split this very large function into distinct sub-parts?

Overly-long, multi-purpose functions are most often the result of poorly or loosely defined functions that have grown large to support the changing needs of the project. In game programming, object update functions in their various incarnations (Update(), Tick(), HandleFrame(), etc.) are common culprits. Functions that have grown this way are usually collections of distinct sections of logic that even the author may have yet to realize as such.

For example, while writing some target-picking logic in the Update() routine of an EnemyShip class, it’s quite natural to think of that logic in terms of simply being part of EnemyShip’s Update logic (which it is.) But, it’s also valid to think of it as part of EnemyShip’s targeting logic as distinct from its movement or appearance logic (which may also happen to be living in the Update() function.) Given a valid form of classification, it’s often wise to develop the class function-wise to match that classification. Consider the following, somewhat contrived, code:

public void EnemyShip::Update( float deltaSeconds )
{
    //Pick target closest target
    Ship* pTargetShip = NULL;
    float distanceToTarget = FLT_MAX;
    for( int i = 0; i < m_TargetList.size(); ++i )
    {
       if( Vector2::Distance( m_MyPosition, m_TargetList[i]->GetPosition() ) < FLT_MAX )
       {
         pTargetShip = m_TargetList[i];
         distanceToTarget = Vector2::Distance( m_MyPosition, m_TargetList[i]->GetPosition() );
       }
    }

    //Move closer to target
    if( pTargetShip)
    {
      Vector2 targetDirection = m_Position - pTargetShip->GetPosition();
      targetDirection.Normalize();
      SetImpulseDirection( targetDirection );
    }
    else
    {
      SetImpulseDirection( Vector2::RandomUnitVector() );
    }

    //Adjust damage state based on HP
    if( m_HP >= MAX_HP * 0.75f )
    {
      m_VisualState = VISUALSTATE_FRESH;
    }
    else if( m_HP >= MAX_HP * 0.5f && m_HP < MAX_HP * 0.75f )
    {
      m_VisualState = VISUALSTATE_HURT;
    }
    else if( m_HP >= MAX_HP * 0.25f && m_HP < MAX_HP * 0.5f )
    {
      m_VisualState = VISUALSTATE_BADLY_HURT;
    }
    else if( m_HP < MAX_HP * 0.25f )
    {
      m_VisualState = VISUALSTATE_CRITICAL;
    }
}

Even at this simple state, this function is already showing signs of confusion of purpose and a proclivity towards growth. It’s easy to imagine this function growing as new ideas, in the throes of prototyping passion, are tried out, modified, discarded and reinstated. It’s probably wise to nip this sort of problem in the bud and try out some code like this on for size:

public void EnemyShip::Update( float deltaSeconds )
{
    Ship* pTargetShip = NULL;

    SelectTarget( pTargetShip );
    MoveShip( pTargetShip );
    UpdateVisualState();
}

private inline void EnemyShip::SelectTarget( Ship* &pTargetShip )
{
    //Pick target closest target
    float distanceToTarget = FLT_MAX;
    for( int i = 0; i < m_TargetList.size(); ++i )
    {
       if( Vector2::Distance( m_MyPosition, m_TargetList[i]->GetPosition() ) < FLT_MAX )
       {
         pTargetShip = m_TargetList[i];
         distanceToTarget = Vector2::Distance( m_MyPosition, m_TargetList[i]->GetPosition() );
       }
    }
}

private inline void EnemyShip:: MoveShip( Ship* pTargetShip )
{
    //Move closer to target
    if( pTargetShip)
    {
      Vector2 targetDirection = m_Position - pTargetShip->GetPosition();
      targetDirection.Normalize();
      SetImpulseDirection( targetDirection );
    }
    else
    {
      SetImpulseDirection( Vector2::RandomUnitVector() );
    }
}

private inline void EnemyShip:: UpdateVisualState()
{
    //Adjust damage state based on HP
    if( m_HP >= MAX_HP * 0.75f )
    {
      m_VisualState = VISUALSTATE_FRESH;
    }
    else if( m_HP >= MAX_HP * 0.5f && m_HP < MAX_HP * 0.75f )
    {
      m_VisualState = VISUALSTATE_HURT;
    }
    else if( m_HP >= MAX_HP * 0.25f && m_HP < MAX_HP * 0.5f )
    {
      m_VisualState = VISUALSTATE_BADLY_HURT;
    }
    else if( m_HP < MAX_HP * 0.25f )
    {
      m_VisualState = VISUALSTATE_CRITICAL;
    }
}

Now, this results in a bit more code, but ultimately it is better organized code. The functions used in Update() explicitly name the sections of logic handled during EnemyShip’s update loop and formally define the information shared between these sections of logic. Now, as further development proceeds on EnemyShip, we have a clear indication of where new logic of particular types should go: targeting logic will find a suitable home in SelectTarget(), additional movement modes will be a nice fit inside MoveShip(). We also have a precedent for calling out to more specialized functions from Update() and building up logic within them. This will make someone think twice about the purpose of their code before simply throwing it into the Update() function. Breaking up the logic into separate functions also protects us from accidental sharing of data between the logic chunks as any shared data will have to be passed as parameters through function calls. As a nod to clean class interface advocates, the private modifier on Update()’s sub-functions keeps them from cluttering up EnemyShip’s public interface.

It’s true that this sort of organization of code doesn’t make a difference in the ultimate behavior of the program. Indeed, if you do it right, there should be absolutely no difference in behavior. It is ultimately about organizing your code and understanding what you are writing as deeply as possible. This is the sort of change you make to head off possible future errors. It’s an investment into the debugging endgame of a project and one that I often wish, after the fact, that I had done more aggressively earlier in development.