A few days ago I ran into Kent Spillern’s post, Private Methods are a Code Smell. Here’s the meat of it:
Private helper methods indicate classes are doing too many things. Moving private helper methods to different classes, including creating new classes if necessary, splits the original responsibilities across multiple classes leading to simpler, better designs.
This post hit really close to home for me because I’m a heavy user of private utility methods. Splitting code up into lots of methods is a form of self-documentation that I advocate. He talks about one misuse of private methods that I don’t indulge in:
Sometimes, private methods are created to split complex logic or processes into small, easily digested pieces. Often, such private methods have gnarly dependencies because they directly access or modify internal state.
I write private methods for the reasons described in the first sentence, but I very rarely do what’s described in the second sentence — write private methods that modify the member variables in a class. Recently, though, I’ve started moving away from private methods. I don’t mind classes with lots of methods, but when you’re writing tests, it’s easiest to deal with small bits of code with known inputs and outputs.
I’ve also gotten used to frameworks like Rails and Kohana that both support the concept of helpers, which are a sort of thowback to procedural programming. I like helpers because they’re easy to test, and because you can use them in any situation you like. The more object-oriented alternative to writing helper or “util” classes is to use superclasses. Classes that share functionality extend the same superclass that contains the methods with shared functionality. This is the approach that the Spring Framework takes.
Even though I’d thought about moving away from private and protected methods, I had never really thought of them as something to be deliberately avoided, but I’m halfway along the path to being convinced.
January 27, 2010 at 8:24 am
I see the point, but I think private methods are exactly intended to break up more complex strings of execution that are used internally, but should not be exposed to the consumer class. It had not occurred to me that this could be an indicator of when to build a sub class. I’ve always relied on Is-A versus Has-A and my own intuition and design needs to do that. I will pay more attention now. Thanks for the post.
January 27, 2010 at 1:32 pm
How does this apply to the practice of factoring out the inner body of a loop into a private method for readability and maintenance purposes?
One implication I suppose could be that internal state should be passed to the private method as a parameter rather than accessed directly. Does that sound reasonable?
January 27, 2010 at 11:13 pm
Interesting thought, not sure how far I buy it. I mean, everything is a code smell by some standards – small APIs! Big APIs! Private methods! Public methods!
I agree that complex manipulation of private state is frequently a mistake. It is of course just global variables by another name, it’s just that those globals are class member variables. However, when you have private state that needs to be manipulated, the only workable approach is private methods that manipulate that state; the question is whether those methods are “kinda like” what you would do for a public interface, or whether they’re trying to do too much.
On passing internal state to private methods – often I find that private class variables are really substituting for method parameters, “saving” you from passing them around. Well, those should usually be local variables in the originating method, and should be passed in. What I do to try to prevent that is to make private methods static by default. That way everything they deal with gets passed in and nothing else is available.