Private Methods Are Code Smell

I notice myself using private methods less and less over the years. Recently I've been helping a team wrangle in some legacy code and see a lot of stuff like this:

@Override
public Thinger doTheThing(String identifier) {
	Gribble gribble = this.gribbleService.find(identifier);
	Pibble pibble = this.pibbleService.findAssociatedPibble(identifier);

	WidgetClientResponse response = this.doWidgetClientCall(gribble, pibble);

	return this.translateWidgetClientResponseToThinger(response);
}

private WidgetClientResponse doWidgetClientCall(Gribble gribble, Pibble pibble) {
	WidgetClientResponseWrapper wrapper = this.widgetClient.call(
		WidgetCallType.FOO,
		gribble.getId(),
		false,
		null,
		pibble.getType(),
		null
	);

	return wrapper.getResponse();
}

private Thinger translateWidgetClientResponseToThinger(WidgetClientResponse response, Gribble gribble, Pibble pibble) {
	Thinger thinger = new Thinger();

	thinger.setAssociation(response.getAssociation());

	double probabilityOfAssociation = this.calculateProbability(gribble.getOccurence(), pibble.getOccurence());

	thinger.setProbabilityOfAssociation(probabilityOfAssociation);

	return thinger;
}

private double calculateProbability(double occurence1, double occurence2) {
	if (occurence1 > occurence2 && occurence2 > RandomTool.nextRand()) {
		return occurence1 * MagicNumbers.ASSOCIATION_CONTINGENT;
	} else {
		return occurence2 - MagicNumbers.ASSOCIATION_CONTINGENT;
	}
}
	

At first glance it doesn't look too bad. doWidgetClientCall() cleans up a messy call to widgetClient, translateWidgetClientResponseToThinger() hides the messiness of translating from WidgetClientResponse to Thinger, and calculateProbability() hides some implementation about, presumably, calculating probability. Honestly, if this was the only code in this part of the system, there were no bugs, and there were no requirements for change then I'd probably just leave it as is. But business concerns are conspiring such that this class needs to be changed.

Approaching this from a legacy-code perspective, the first thing I don't like is the slight difficulty in testing the doTheThing() method. Upon reading it's easy to see we'll need to mock out the GribbleService and PibbleService but we have to actually crack open doWidgetClientCall() before learning we'll also need to mock the WidgetClient. If I have control over the WidgetClient I might consider cleaning up the method call, removing nulls and flag args, and giving it a better name than call() but unfortunately we do not own that code. I could extract it into a WidgetService to make it easier to mock and ultimately have it contain only the parameters that I care about, but for now I think inlining it will be fine:

		@Override
		public Thinger doTheThing(String identifier) {
			Gribble gribble = this.gribbleService.find(identifier);
			Pibble pibble = this.pibbleService.findAssociatedPibble(identifier);
		
			WidgetClientResponse response = this.widgetClient.call(
				WidgetCallType.FOO,
				gribble.getId(),
				false,
				null,
				pibble.getType(),
				null
			)
				.getResponse();
		
			return this.translateWidgetClientResponseToThinger(response);
		}
		
		WidgetClientResponse doWidgetClientCall(Gribble gribble, Pibble pibble) {
			WidgetClientResponseWrapper wrapper = this.widgetClient.call(
				WidgetCallType.FOO,
				gribble.getId(),
				false,
				null,
				pibble.getType(),
				null
			);
		
			return wrapper.getResponse();
		}
		

That's a bit better. Sure, aesthetically it's less pleasing but with a little whitespace around the client call it's easy to keep visually quarantined from the surrounding code. The next thing I don't like from a testing standpoint is calculateProbability(). I can't reliably test the two branches due to RandomTool.nextRand(). Also it's confusing as shit. What's the range of that random number? What's an "occurence"? And what is an "Association Contingent"? Testing this through the public API of the class will be a pain due to the large setup required. We could elevate the method access to test it directly but that's ugly¹. But mostly this doesn't feel like it belongs with the rest of the code, put here because the original author found it expedient or didn't feel it warranted the creation of another class. Figuring out exactly what's going on here is beyond the scope of this exercise so I will simply extract it to ProbabilityCalculator and call it done.

private Thinger translateWidgetClientResponseToThinger(WidgetClientResponse response, Gribble gribble, Pibble pibble) {
	Thinger thinger = new Thinger();

	thinger.setAssociation(response.getAssociation());

	double probabilityOfAssociation = this.calculateProbability(gribble.getOccurence(), pibble.getOccurence());
	double probabilityOfAssociation = this.probabilityCalculator.calculate(gribble.getOccurence(), pibble.getOccurence());

	thinger.setProbabilityOfAssociation(probabilityOfAssociation);

	return thinger;
}

private double calculateProbability(double occurence1, double occurence2) {
	if (occurence1 > occurence2 && occurence2 > RandomTool.nextRand()) {
		return occurence1 * MagicNumbers.ASSOCIATION_CONTINGENT;
	} else {
		return occurence2 - MagicNumbers.ASSOCIATION_CONTINGENT;
	}
}
		

Getting better but I don't like that translateWidgetClientResponseToThinger() calls probabilityCalculator.calculate() because it makes the code hard to trace through. But that's easy enough to fix:

@Override
public Thinger doTheThing(String identifier) {
	Gribble gribble = this.gribbleService.find(identifier);
	Pibble pibble = this.pibbleService.findAssociatedPibble(identifier);
	double probabilityOfAssociation = this.probabilityCalculator.calculate(gribble.getOccurence(), pibble.getOccurence());

	WidgetClientResponse response = this.widgetClient.call(
		WidgetCallType.FOO,
		gribble.getId(),
		false,
		null,
		pibble.getType(),
		null
	)
		.getResponse();

	return this.translateWidgetClientResponseToThinger(response, probabilityOfAssociation);
}

private Thinger translateWidgetClientResponseToThinger(WidgetClientResponse response, Gribble gribble, Pibble pibble double probabilityOfAssociation) {
	Thinger thinger = new Thinger();

	thinger.setAssociation(response.getAssociation());

	double probabilityOfAssociation = this.calculateProbability(gribble.getOccurence(), pibble.getOccurence());
	thinger.setProbabilityOfAssociation(probabilityOfAssociation);

	return thinger;
}
		

And that's pretty good. I like translateWidgetClientResponseToThinger() now that it's a pure function with no state (global or otherwise) and the translation from domain-object to framework-object has to be done somewhere. It doesn't belong with WidgetClientResponse because that shouldn't know about framework things and Thinger most likely can't know about WidgetClientResponse. There could be a separate class that does the translation but that feels like overkill and besides, it's sort of the raison d'etre for this class; without it we'd only be testing that the things returned from one mock were input to another mock.

Hot Take: Private methods are a code smell and should be avoided until necessary. They have their place but most usages I come across in the wild are at best wrapping up behavior that should be extracted to another class and at worst a Bag Of Code.

¹ To be clear I'm not above elevating method access to test, and will under extreme duress, but I tend to leave it as a last resort second only to not testing.