Code Style & Taste
A blog where I share thoughts on code and practices

Functions Should Be Independent

Let's start with an example and build on it. At what point do you think something went wrong? If you want to skim the code you can read the comments.

First, we'll launch a script and return true if it runs without errors.

boolean runScript() {
	var process = new MyProcess("script");
	process.setArgs("--default --args".split(' '));
	process.run();
	process.wait();
	return process.exitCode == 0;
}

Sometimes the script fails because of the directory. Try an alternative directory if that happens.

boolean runScript() {
	var process = new MyProcess("script");
	process.setArgs("--default --args".split(' '));
	process.run();
	process.wait();
	
	if (process.exitCode != 0) { // New code
		process.chdir(getAlternativeDirectory());
		process.run();
		process.wait();
	}
	return process.exitCode == 0;
}

We find out users are running an old version of the script. As a backup, we'll try the script using older flags

// New code
void runOldStyle(MyProcess process) {
	process.setArgs("--old --style".split(' '));
	process.run();
}

boolean runScript() {
	var process = new MyProcess("script");
	process.setArgs("--default --args".split(' '));
	process.run();
	process.wait();

	if (process.exitCode != 0) { // New code
		runOldStyle(process);
	}

	if (process.exitCode != 0) {
		//Is it intentional that this now uses the older style args?
		process.chdir(getAlternativeDirectory());
		process.run();
		process.wait();
	}
	return process.exitCode == 0;
}

We notice a case where we get less desirable results because we use the older style args before we try changing the directory. So let's swap the order.

boolean runScript() {
	var process = new MyProcess("script");
	process.setArgs("--default --args".split(' '));
	process.run();
	process.wait();

	//Swapped, no longer uses the older style args
	if (process.exitCode != 0) {
		process.chdir(getAlternativeDirectory());
		process.run();
		process.wait();
	}

	//Incorrect directory
	if (process.exitCode != 0) {
		runOldStyle(process); 
	}

	return process.exitCode == 0;
}

We're at the mercy of our test and maybe code review to discover our code is broken whenever a user is running with the old script. Before the change the script always succeeded with the old style args in the correct directory, it never ran under the alternative directory. Now that it's running in the wrong directory it always fails. If no one reports the directory being the error whomever swapped the order may have no idea why it broke.

Was there a change that was obviously wrong? Could we have made the bug more obvious or avoid it completely? At what point should we have redesigned MyProcess? I would say MyProcess was flawed from the start.

Functions Should Be Independent

Functions should do what you expect regardless of what was called before it. Only parameters and configurations passed into the constructor should affect behavior. Needing to set options before calling a function is error-prone and frustrating when you're refactoring or busy fixing a bug. Here are two ways we can fix this

boolean runScript() {
	var process = new MyProcess("script", "--default --args".split(' ')); //We no longer can change args
	process.wait();
	return process.exitCode == 0;

	//alternatively
	var process = new MyProcess("script");
	process.run("--default --args".split(' ')); //Pass args into run
	process.wait();
	return process.exitCode == 0;
}

I prefer the first, even if our use case would like to re-run our script with different args. With the second you may ask, does wait depend on run? No, you could call run 0 or 100 times before you first call wait. It doesn't matter.

Here's how I'd write the previous code

boolean runScript() {
	var script = "script";
	var defaultArgs = "--default --args".split(' ');
	var process = new MyProcess(script, defaultArgs);
	var exitCode = process.wait();
	
	if (exitCode != 0) {
		//Function overload to set the directory
		process = new MyProcess(script, defaultArgs, getAlternativeDirectory());
		process.wait();
	}

	//Yes I would have wait return the exit code
	//and allow it to be accessible from the object
	return process.exitCode == 0;
}
Next is handling the old style. Two differences are 1. We're no longer passing in MyProcess 2. We are using defaultArgs with the alternative directory. This is more clear than the original version which may not have intentionally used the old style args. 
MyProcess runOldStyle(string script) {
	var process = new MyProcess(script, "--old --style".split(' '));
	process.wait();
	return process;
}
boolean runScript() {
	var script = "script";
	var defaultArgs = "--default --args".split(' ');
	var process = new MyProcess(script, defaultArgs);
	process.wait();

	if (process.exitCode != 0) {
		process = runOldStyle(script);
	}

	if (process.exitCode != 0) {
		//Clearly using defaultArgs, no longer using old style on accident
		process = new MyProcess(script, defaultArgs, getAlternativeDirectory());
		process.wait();
	}
	return process.exitCode == 0;
}
Now when we swap the order nothing will break 
boolean runScript() {
	var script = "script";
	var defaultArgs = "--default --args".split(' ');
	var process = new MyProcess(script, defaultArgs);
	process.wait();

	//This is moved up
	if (process.exitCode != 0) {
		process = new MyProcess(script, defaultArgs, getAlternativeDirectory());
		process.wait();
	}

	if (process.exitCode != 0) {
		process = runOldStyle(); //Now this doesn't run with the alt directory
	}

	return process.exitCode == 0;
}

If we wrote the code in this style, swapping the functions would have never introduced a bug. If the situation is different and we should be using the old style args with the alternative directory, the bug would be clearer since we can see what args are being passed into run without carefully reading the lines and function calls before it.

Low Level APIs

This technique is great when you need to use low level APIs. Typically people reach for something high level but sometimes it doesn't make sense. If you're writing a chat app for a phone where you can send files, you could implement sending a file by putting the file and filename into an object, json encode it, and send the 1gb+ file in one big write call. It'd work since most phones have enough ram to handle that, but it'd be awkward not being able to send messages while the app is sending the file. Likewise, if you're drawing some text to screen, you don't need to ship a GUI runtime to make development more pleasant with high level abstractions, sometimes the code to set up a library can be more code than your own implementation (I've seen this happen more than once.)

Let's use rendering as an example. If you seen old style opengl code you'll know setting the color of a triangle is a different step than drawing a triangle. Here is an example of a fake api that starts with ui_

void renderScreen() {
	ui_setColor(backgroundColor);
	ui_clearScreen();
	int x=10, y=10;
	
	ui_setColor(menuColor)
	renderText("File Edit Etc", x, y);
	
	x = screenWidth/2;
	ui_setColor(iconColor);
	ui_renderIcon(someIcon1, x, y);
	if (extraMenuItem) {
		x = width-250;
		ui_renderIcon(anotherIcon, x, y);
		x += 50;
		if (tooltip) {
			ui_setColor(tooltipColor);
			renderText("tooltip here")
			x += 100;
		}
		if (anotherTip) {
			//oops, if this is ever true when tooltip is false this will be the wrong color
			renderText("additional tip");
			x += 100
		}
	}
	//more
}

The API might be simple, it may be close to what we want, and we definitely don't need an abstraction layer, but if our code became complicated enough that we had to fix a dozen graphical bugs every release, we may want a util function to prevent a handful of these from happening.

void renderText(string text, int x, int y, Color color); //We add color to our parameters
void setColor(Color color) { if (color == currentColor) return; currentColor = color; ui_setColor(color); }
//We almost always want the color to be the default icon color
void renderIcon(Icon icon, int x, int y, Color color=iconColor)
	{ setColor(color); ui_renderIcon(icon, x, y); }
void renderScreen() {
	ui_setColor(backgroundColor); //We still use the original api
	ui_clearScreen();
	int x=10, y=10;
	renderText("File Edit Etc", x, y, menuColor);
	x = screenWidth/2;
	renderIcon(someIcon1, x, y);
	if (extraMenuItem) {
		x = width-250;
		renderIcon(anotherIcon, x, y);
		x += 50;
		if (tooltip) {
			renderText("tooltip here", tooltipColor)
			x += 100;
		}
		if (anotherTip) {
			renderText("additional tip", tooltipColor);
			x += 100
		}
	}
	//more
}

Graphical bugs usually aren't a big deal. It can be annoying when you or a user triggers an untested combination and accidentally break the UI. If you're trying to improve the layout it can be frustrating constantly adding code to fix elements because they assumed what was being drawn before it.

If you ever find yourself writing a class where you must or should call a function before another, think of this article and all the bugs it could produce. If there's a little overhead to allow functions to be independent you may want to take it. It is certainly better than everyone avoiding the API. If you're using an API with a lot of implicit state and it's becoming a problem, see if you can solve it by writing utility functions that behave independently before writing an entire abstraction layer.

If you like to read more, you can follow this blog via rss or subscribe via email