Let The Refactoring Begin - richardjwild/arctracker GitHub Wiki

There was a lot that I perceived wrong with this codebase, the most serious failings being (but by no means the only ones):

  1. Huge functions
  2. Long argument lists
  3. Code being switched on/off all over the place with #ifdef directives
  4. Functions returning error codes everywhere, obscuring their intent
  5. Generally poor naming
  6. All code contained in four large files

So I decided to pick on the function named initialise_phase_incrementor_values in initialise.c to begin with, because on the one hand it was quite small and easy to understand, and on the other hand it still exhibited a number of the failings noted above:

return_status initialise_phase_incrementor_values (
	long **p_phase_incrementors,
	unsigned int *p_periods,
	long p_sample_rate)
{
	return_status retcode = SUCCESS;
	int period;
	long *phase_incrementors_ptr;

	if ((*p_phase_incrementors = (long *)malloc(2048 * sizeof(long))) == NULL) {
		fprintf(stderr,"Cannot allocate memory for phase incrementors array\n");
		retcode = MEMORY_FAILURE;
	} else {
		phase_incrementors_ptr = *p_phase_incrementors;

		for (period=1; period</*1021*/ 2048; period++) {
			/* don't ask me how this conversion works, I *
			 * copied it from the tracker player source */
			*phase_incrementors_ptr = (3575872.0/((double)period * (double)p_sample_rate)) * 60000.0;
			phase_incrementors_ptr++;
		}
	}

	return (retcode);
}

The biggest problem here is that the function returns an error code, when really its job is to return an array of phase increments. I want it to return the array directly, but instead it is forced to accept a pointer to a pointer (long **p_phase_incrementors) in order to pass its return value back. I hate that. The point of the error code is to communicate that malloc failed, but if that happens then the only thing the program can do is bomb out. Surely we should just bomb out instead. But the first thing I decided to fix is the phase_incrementors_ptr variable. Why on earth did I do that, instead of simply treating it as an array?

So I did that.

The next thing I did was remove all the comments. I was particularly concerned by the one that said:

/* don't ask me how this conversion works, I *
 * copied it from the tracker player source */

Well if I don't know, who else is going to? So I determined to try to make sense of the magic numbers there. But the next thing I did was break up that if statement by separating the call to malloc() from the check for success. And then for good measure, I inlined the declaration of the loop counter variable because this isn't the 20th century any more, for heaven's sake.

The next thing I did was add an error() function that exits the program, so I do not need to return error codes from all my functions any more. This enabled me to rewrite the signature of initialise_phase_incrementor_values() to return an array, thus allowing the awkward pointer-to-pointer argument to be removed.

Now it was time to move this function out - one of the main things I wanted to do in this refactoring effort was to break the codebase up into smaller sourcefiles. So I created a new file resample.c and moved the function into it. Why did I call it resample.c? I knew that the phase incrementors were used for sample rate conversion, in order to play back the samples at different pitches. This is often referred to in audio engineering as resampling.

Next I extracted a function to calculate a phase increment and it was at this point I noticed that the p_periods argument was not being used at all! So I deleted it.

I took the opportunity next to rename initialise_phase_incrementor_values to calculate_phase_increments and also extracted some more constants. It was still bothering me that I did not know what the meaning was of those magic numbers 60000.0 and 3575872.0, nor why the calculated array has 2047 elements (not 2048 as the code originally implied). But first, I created a function allocate an array in a new file heap.c and used that instead of calling malloc directly, because mixing levels of abstraction is one of the principal problems that prevent code being clear.

That aside, to understand this calculation, I had to go and look where the results were being used. That was in get_new_note in play_mod.c:

/* get period and phase incrementor value */
phase_incrementors_ptr = p_phase_incrementors;
periods_ptr = p_periods;

if (p_module_type == TRACKER) {
	periods_ptr += (p_current_event->note + 12); /* desktop tracker has greater chromatic range */
} else {
	p_current_voice->note_currently_playing += (13 - sample_info_ptr->note);
	periods_ptr += (p_current_event->note + 13 + (13 - sample_info_ptr->note));
	p_current_voice->volume = ((p_current_voice->volume + 1) << 1) - 1; /* desktop tracker volumes from 0..127 not 0..255 */
}
phase_incrementors_ptr += *periods_ptr;

p_current_voice->period = *periods_ptr;
p_current_voice->target_period = *periods_ptr;
p_current_voice->phase_increment = *phase_incrementors_ptr;

Now that is very far from clean code, but the basic gist of it was this. The phase_incrementors_ptr pointer initially points at the beginning of the array we have calculated. It then looks up the "period" value for the note to be played. Each different note has an associated period, which is inversely related to the frequency; that makes it sound like the wavelength, which it certainly is not, although those are the right lines to be thinking along if you are. The period value is then used to look up the phase increment value from the array we have calculated. This meant two things to me:

  1. I was wrong to rename the loop counter variable i in phase_incrementors_ptr, it really is a period.
  2. I should write a function in resample.c that returns the phase increment for a given period.

And that is what I did here. None of this got me any closer to understanding exactly why those magic numbers have the values they do, but I become more sanguine about them. I still did not know what exactly the "sample period" numbers meant, other than that they relate inversely to pitch, but the magic numbers converting them to the phase increments clearly are magic: they must be the right value or the music will play at the wrong pitch, end of story. Maybe later I will understand the sample periods better. In the meantime, I changed some names. For my first bit of refactoring, I was quite happy. When I was done, resample.c looked like this:

#include <stdlib.h>
#include "heap.h"

#define PITCH_QUANTA 2047
#define PHASE_INCREMENT_CONVERSION 60000L * 3575872L

long* phase_increments;

long phase_increment(int period, long sample_rate)
{
    return PHASE_INCREMENT_CONVERSION/(period * sample_rate);
}

void calculate_phase_increments(long sample_rate)
{
    phase_increments = (long*) allocate_array(PITCH_QUANTA, sizeof(long));

    for (int period=1; period<=PITCH_QUANTA; period++)
        phase_increments[period - 1] = phase_increment(period, sample_rate);
}

long phase_increment_for(int period)
{
    return phase_increments[period];
}

Previous: Fixing the Warnings | Next: Resampling

⚠️ **GitHub.com Fallback** ⚠️