Smaller functions for readable code

When I was teaching myself C++ programming with programs like Black Shades and Lugaru, it hadn't even occured to me to think about how readable the code was. All I cared about was making it work as fast as possible, so there were a lot of duplicated code sections, global variables, and mixed conventions. The game worked, but it was difficult to maintain, and difficult to port to other platforms.

One technique I'm using for Overgrowth code is breaking it down into clear, bite-sized functions. This allows the function name to take the place of comments, and hides unnecessary implementation details. To show why this is important, let's take an incomprehensible code snippet from Black Shades, that I wrote 8 years ago, and break it down until it's readable:

---

void HandleMenuButtonOne() {
if((Button()&&mouseoverbutton==1&&!gameinprogress&&!oldbutton)||
                                                    !mainmenuness){
    if(environment==rainy_environment)
        alSourcePlay(gSourceID[rainsound]);
    if(environment!=rainy_environment)
        alSourcePause(gSourceID[rainsound]);
    alSourceStop(gSourceID[whichsong]);
    alSourcef(gSourceID[whichsong], AL_MIN_GAIN, 0);
    alSourcef(gSourceID[whichsong], AL_MAX_GAIN, 0);
    if(person[0].whichgun==knife)whichsong=knifesong;
    if(person[0].whichgun!=knife)whichsong=shootsong;
    if(type==zombie_type)whichsong=zombiesong;
    alSourcef(gSourceID[whichsong], AL_PITCH, 1);
    alSourcePlay(gSourceID[whichsong]);
    alSourcef(gSourceID[whichsong], AL_MIN_GAIN, 1);
    alSourcef(gSourceID[whichsong], AL_MAX_GAIN, 1);
    flashamount=1; 
    flashr=1;flashg=1;flashb=1;
    mainmenu=0;
    alSourcePlay(gSourceID[soulinsound]);
    mission=0;
    InitGame();
    gameinprogress=1;
}

if((Button()&&mouseoverbutton==1&&gameinprogress&&!oldbutton)||
                                                    !mainmenuness){
    flashamount=1;
    flashr=1;flashg=1;flashb=1;
    mainmenu=0;
    MoveMouse(oldmouseloc.h,oldmouseloc.v,&mouseloc);
    if(environment==rainy_environment)
        alSourcePlay(gSourceID[rainsound]);
    if(environment!=rainy_environment)
        alSourcePause(gSourceID[rainsound]);
    alSourceStop(gSourceID[whichsong]);
    alSourcef(gSourceID[whichsong], AL_MIN_GAIN, 0);
    alSourcef(gSourceID[whichsong], AL_MAX_GAIN, 0);
    if(person[0].whichgun==knife)whichsong=knifesong;
    if(person[0].whichgun!=knife)whichsong=shootsong;
    if(type==zombie_type)whichsong=zombiesong;
    alSourcePlay(gSourceID[whichsong]);
    alSourcef(gSourceID[whichsong], AL_MIN_GAIN, 1);
    alSourcef(gSourceID[whichsong], AL_MAX_GAIN, 1);
    alSourcePlay(gSourceID[soulinsound]);
    if(visions)alSourcePlay(gSourceID[visionsound]);
} 
}

---

This is quite a mess. We can tell from the function name that it "handles menu button one," but it's impossible to see anything else -- it's a tangle of duplicated low-level implementation details. To clear this up, let's start by finding the most obvious duplicated code segments (code that appears in both of the big 'if' statements) and extracting them into separate functions with clear names.

---

void FlashWhite() {
    flashamount=1;
    flashr=1;
    flashg=1;
    flashb=1;
}

void HandleRainSound(){
    if(environment==rainy_environment){
        alSourcePlay(gSourceID[rainsound]);
    } else {
        alSourcePause(gSourceID[rainsound]);
    }
}

void StopSoundtrack(int song_id) {
    alSourceStop(gSourceID[song_id]);
    alSourcef(gSourceID[song_id], AL_MIN_GAIN, 0);
    alSourcef(gSourceID[song_id], AL_MAX_GAIN, 0);
}

int GetLevelSoundtrackID() {
    int level_song;
    if(person[0].whichgun==knife){
        level_song = knifesong;
    } else {
        level_song = shootsong;
    }
    if(type==zombie_type){
        whichsong=zombiesong;
    }
    return level_song;
}

void PlaySoundtrack(int song_id) {
    alSourcef(gSourceID[song_id], AL_PITCH, 1);
    alSourcePlay(gSourceID[song_id]);
    alSourcef(gSourceID[song_id], AL_MIN_GAIN, 1);
    alSourcef(gSourceID[song_id], AL_MAX_GAIN, 1); 
}

void PlaySound(int sound_id){
    alSourcePlay(gSourceID[sound_id]);
}

void HandleMenuButtonOne() {
    if((Button()&&mouseoverbutton==1&&!gameinprogress&&!oldbutton)||
                                                    !mainmenuness){
        HandleRainSound();
        StopSoundtrack(whichsong);
        whichsong = GetLevelSoundtrackID();
        PlaySoundtrack(whichsong);

        FlashWhite();
        mainmenu=0;
        PlaySound(soulinsound);
        mission=0;
        InitGame();
        gameinprogress=1;
    }
    if((Button()&&mouseoverbutton==1&&gameinprogress&&!oldbutton)||
                                                    !mainmenuness){
        FlashWhite();
        mainmenu=0;
        MoveMouse(oldmouseloc.h,oldmouseloc.v,&mouseloc);
        HandleRainSound();
        StopSoundtrack(whichsong);
        whichsong = GetLevelSoundtrackID();
        PlaySoundtrack(whichsong);
        PlaySound(soulinsound); 
        if(visions){
            PlaySound(visionsound);
        }
    }
}

---

Now HandleMenuButtonOne() is a much smaller wall of text, with relatively informative function calls. However, the big picture is still muddied by implementation details -- we can't just look at it and see what's happening. Let's extract some more functions, and move some duplicated code out of the 'if' statements. Now we get this:

---

void SetupBackgroundSounds(){
    HandleRainSound();
    StopSoundtrack(whichsong);
    whichsong = GetLevelSoundtrackID();
    PlaySoundtrack(whichsong);
}

void PlayButtonClickSpecialEffect(){
    PlaySound(soulinsound);
    FlashWhite();      
}

bool WasMouseButtonClicked() {
    if(Button() && !oldbutton) {
        return true;
    } else {
        return false;      
    }
}

void HandleMenuButtonOne() {
    if(mouseoverbutton != 1 || !WasMouseButtonClicked()){
        return;
    }

    PlayButtonClickSpecialEffect();

    if(!gameinprogress||!mainmenuness){
        mission=0;
        InitGame();
        gameinprogress=1;
    }

    if(gameinprogress||!mainmenuness){
        MoveMouse(oldmouseloc.h,oldmouseloc.v,&mouseloc);
        if(visions){
            PlaySound(visionsound);
        }
    }

    SetupBackgroundSounds();
    mainmenu=false;
}

---

Now HandleMenuButtonOne() is starting to make some sense. If we focus on it for a minute or two we can see that it resumes a game in progress, or starts a new game if there is no game in progress. However, it's still not as readable as it should be -- we want our functions to be clear enough that another programmer could just read it like a book and instantly see what is going on. The best we can do here is to extract the 'new game' and 'resume game' functions, and add some brief comments for clarity.

---

void StartNewGame() {
    mission=0;
    InitGame();
    gameinprogress=1;
}

void ResumeGame() {
    MoveMouse(oldmouseloc.h,oldmouseloc.v,&mouseloc);
    if(visions){
        PlaySound(visionsound);
    }
}

void HandleMenuButtonOne() {
    if(mouseoverbutton != 1 || !WasMouseButtonClicked()){
        return; // Mouse is not clicking on this button
    }

    PlayButtonClickSpecialEffect();

    if(!mainmenuness){
        StartNewGame();
        ResumeGame();
    } else if(gameinprogress){
        ResumeGame();      
    } else {
        StartNewGame();
    }

    SetupBackgroundSounds(); // Switch from menu song to level song
    mainmenu=false;
}

---

Now it is pretty readable! We still have no way of knowing what 'mainmenuness' is supposed to be, or why both branches are executed if it is false -- it looks like it may be some kind of bug or legacy development feature. This shows one of the advantages of breaking down code like this -- it would have been impossible to spot this stray variable in the original mess. If we track this down and it turns out to be unused, we can delete it. The lowercase variable names and whitespace usage here are still not great, but the structure is now very easy to follow:

---

void HandleMenuButtonOne() {
    if(mouseoverbutton != 1 || !WasMouseButtonClicked()){
        return; // Mouse is not clicking on this button
    }

    PlayButtonClickSpecialEffect();

    if(gameinprogress){
        ResumeGame();      
    } else {
        StartNewGame();
    }

    SetupBackgroundSounds(); // Switch from menu song to level song
    mainmenu=false;
}

---

If you're a veteran coder, you may be thinking, "Everyone knows that smaller functions are better, why bother writing about that here?" Maybe so, but I know that I've never seen a computer science class that actually teaches good coding habits, or why they matter, so maybe this will help someone. Would you be interested in reading about more coding strategies like this?