Simplifying code using states

Lately I have been refactoring czPlayer to add more features and make future maintenance easier.
Some parts of the code are almost 10 years old, when I first started playing with sound coding.
Needless to say, my code style and quality has improved a lot in 10 years, and it’s a funny thing to look at code this old. 🙂
As I was refactoring the code, I found a lot of ambiguities and redundancies caused by member variables added over time.
Let me explain….
I had a class responsible for MOD processing, and some of the member variables were:

class MODModule
{
	// ...
	bool m_loaded;
	bool m_playing;
	bool m_paused;
	bool m_reachedEnd;
};

What’s the problem with this approach ?
Let’s see…
Suppose I want to call methods on the MOD object (e.g: Pause(), Resume(), etc)
I need to make some checks in each method.

To pause…

if (!m_loaded || m_playing || m_paused)
	return SOME_ERROR;
m_pause = true;
// ... do pause stuff

Then to resume…

if (!m_loaded || !m_paused)
	return SOME_ERROR;
m_paused = false;
// ... do resume stuff

When the MOD finishes playing, I needed something like this…

// ... some code that checks for end
m_reachedEnd = true;
//ups... How about m_playing, m_paused?
//We need to change their values, so we don't break code elsewhere
m_playing = false;
m_pause = false;

And similar things when playing, stopping, etc.
See the problems?

We need checks for multiples variables
We need to make sure the variables have valid values in relation to each other
What if we want to add an extra state? For example, “bool m_fading”. We would need to examine a lot of code to check how the other variables are used, and hardwire this new one somewhere. Every time you need to add an extra variable, you’ll need to check more and more code to make sure everything is ok.

Now look at the meaning of the variables. They are mostly mutually exclusive.

If it’s playing, it can’t be paused and hasn’t reached the end.
If it’s paused, it’s not playing, and hasn’t reached the end yet.
… and so on….

The more data members a class has, the harder it gets to keep the object in a valid state, so, the less data to manage, the better. Less maintenance and less bugs.

One technique I use a lot when trying to simplify code is to look at it with state diagrams whenever possible.
Here it is what happens (kind of) with instances of the MODModule class:

I can easily translate it to code with enums.

enum MODState
{
    MOD_STATE_NOT_LOADED,
    MOD_STATE_STOPPED,
    MOD_STATE_PLAYING,
    MOD_STATE_PAUSED,
    MOD_STATE_REACHED_END
};

class MODModule
{
    // ...
    MODState m_state; // Now we have only one variable
};

This approach is a lot easier to maintain, and less error prone.
Now the code in most functions become something like this:

To play…

if (m_state!=MOD_STATE_STOPPED)
    return SOME_ERROR;
m_state = MOD_STATE_PLAYING;
// ... do play stuff

To pause…

if (m_state!=MOD_STATE_PLAYING)
    return SOME_ERROR;
m_state = MOD_STATE_PAUSED;
// ... do pause stuff

To resume…

if (m_state!=MOD_STATE_PAUSED)
    return SOME_ERROR;
m_state = MOD_STATE_PLAYING;
// ... do resume stuff

To stop…

// We cannot stop if it's not even loaded yet, or it's already stopped
if (m_state==MOD_STATE_NOT_LOADED || m_state==MOD_STATE_STOPPED)
    return SOME_ERROR;
m_state = MOD_STATE_STOPPED;
// ... do stop stuff

The code may look similar, but it’s a lot less error prone, easier to follow and maintain.
Looking at the State Diagram, I could easily squeeze in a “Fading” state and understand the conditions to enter or leave that state.

As a bonus, using enums will enforce more rules and help you out with errors the compiler might catch. Always prefer compile-time errors over run-time errors. 😉

Share this: