A case against class methods

Situation

You have to take care of a bug report: it seems like for some customers the screen turns red when a customer clicks the "unlock" button. The class is quite ungainly, you were supposed to refactor it, but oh well here we are. You have just discovered the event callback function:

class MyOvergrownSemiAbstractController {

	// Row 3:
	// Lots of variables, at least 50

	// Rows 65-373
	// Lots of code ...

	// Row 374:
	onButtonUnlockClick(optionalBoolean) {
		this.configureState(5);
		if(optionalBoolean) this.setSmurfs();
		return this.getSmurfX();
	}
}

Complexity

At a first glance, this seems quite nice. If we assume that Smurf actually is a meaningful word in this project, the code is split info different descriptive functions. Each function has zero or one argument. This is all according to the style advocated in Clean Code.

However, appearances can be deceiving. Remember, the class owns 50 variables. This means that the complexity of configureState is not only 1 input variable; the complexity is 1 input variable and 50 class variables. Even worse, it could potentially modify 50 of those. We wouldn't know without inspecting each and every function. But if we have to actually look at every function, then what was the point of putting the code in separate functions? Functions are supposed to be abstractions, and if you always have to look through them they are quite meaningless abstractions.

Maybe, the problem with the code is simply that setSmurfs actually has to be run before configureState, to set variable 32 to the correct value. There is however no way for us to know that by looking at onButtonUnlockClick.

Purity

A pure function is only allowed to read from its input variables. It is not allowed to modify any variables outside its own body. This means that a pure function pretty much always needs to have a return value (in order to be meaningful). The benefit of pure functions is that they are predictable. For the same set of input variables, the return value will always be the same.

Mathematical functions are always pure:

// This will always equal -2.4. Even in a bloated class ;-)
Math.min(-2.4, 73)

Rewriting our event-callback in a pure fashion could look like the following.

onButtonUnlockClick(optionalBoolean) {
	let smurfReadiness = this.getState(5);
	let extraSmurfs = optionalBoolean
		? this.setSmurfs()
		: []
	return this.getSmurfX(smurfReadiness, extraSmurfs);
}

Notice how we replaced the scary configureState with a getState. Using "get" is a subtle hint to the person reading the code that the function will not modify any data.

Also, it now is clear that the call to getSmurfX is dependent on the other two.

Breaking free from the class

In some cases, it might be correct to be even more zealous. If a function is pure, why should we keep it as a class method? If a function call is not prefixed with .this, that is virtually a guarantee that no class variables will be read nor written to.

class BigUglyClass {
	onButtonUnlockClick(optionalBoolean) {
		let smurfReadiness = this.getState(5);
		let extraSmurfs = optionalBoolean
			? this.setSmurfs()
			: []
		//return this.getSmurfX(smurfReadiness, extraSmurfs);
		return getSmurfX(smurfReadiness, extraSmurfs);
	}
}

function getSmurfX(smurfReadiness, extraSmurfs) {
	// ...
}

Notice how it suddenly got much easier to create automated tests for getSmurfX-function. We just need to mock some data for smurfReadiness and extraSmurfs.