Arguments - richardjwild/arctracker GitHub Wiki
Nowadays I think, the fewer parameters a function has, the better. The ideal number is zero, but one is good. Two is okay, three acceptable, and if there are more than three I feel I should really rethink the design. Suffice to say that Arctracker violated this rule of thumb pretty much everywhere. C may lack facilities available in object-oriented languages to keep the function parameter counts to a minimum, but in Arctracker the parameter passing was still excessive. It was like I had internalised the rule that global variables are a Bad Thing, but instead I had opted to pass everything around the program from function to function in parameter values. Therefore functions had parameters not because they needed the data themselves but because they needed to pass the data to another function that did. What I had not realised was that the encapsulation available in C compilation units mean you can store global state and mediate access to it through public functions. They act like objects, except that there is only one instance of each. I know that Singletons are considered evil, but a singleton object is better than no object.
The table of sample periods was the most egregious example of a data structure passed from function to function, so I created a module for this and mediated access to it through a function called period_for_note. This then enabled me to remove a parameter from several functions at once. This brought a couple of functions down to the magic three arguments limit, so I squashed their definitions because they now comfortably fitted on one line. While I was there, I noticed that a variable was defined identically on both sides of an if statement which is stupid, so I whipped that outside, and also I extracted a couple of functions and deleted some associated comments.
Getting started on cleaning something up tends to motivate me to continue (I am the same with housework) and I took the opportunity to extract a variable to hold the value of current_pattern_line[channel] which was being referenced in many places. I then gave the same treatment to voice_info[channel] as well. Now that set_portamento_target and trigger_new_note had been reduced to triadic functions I could collapse their invocations to one line each and I removed the braces from the if statement, because that is my preference for one-line code blocks. (Lots of people don't like this but you are perfectly entitled to do it). In the interests of having self-documenting code I extracted a function to test whether a portamento command was happening and similarly extracted a function to test whether a sample was out of range (as explained earlier, this was being used as a note-off command).
I heartily disliked the prefix increment inside an if statement of the tick counter, so I moved that. I also renamed the on_event boolean variable to new_event although I planned to remove it later because it was being passed as a function argument, and boolean parameters are a code smell. Finally, I refactored the play_module function so that it would only loop through the channels once, rather than twice. All this work made the function considerably cleaner than before, although still far from acceptable.
Now it was time to tackle a big change. The program arguments were another data structure being passed around from function to function that I wanted to extract and make global, so I started by creating a configuration module with a cleaned up version of the code to parse the command line arguments, although the code was not hooked up yet. I then introduced it a bit at a time, starting with the module filename when loading the file, and then when initialising the audio system. This seemed to work okay and it gave me to confidence to fully plug the configuration module into play_mod.c function, which enabled me to remove the p_args parameter from the play_module function. The old get_arguments function in initialise.c was now unused so I removed it, which also allowed me to simplify the main method and remove some unused things.
Now I was on a roll with removing unnecessary things, so I went further and removed the arguments from the initialise_oss and initialise_alsa functions. There were a few unused include directives in arctracker.c so they went as well. Finally I added exit(EXIT_SUCCESS) to the main function so that the Portability Police will not come knocking on my door.