Are private methods in general a code smell?

Question

In the sense that a code smell is an indicator of a potential need for refactoring are private methods a code smell?

I was looking at some of my own code and it dawned on me that many of my public methods don't actually do anything other than consolidate private methods of the same class. Furthermore, none of the private methods relied on any member variables.

This got me thinking and I started looking through other classes. Everywhere I looked was the same pattern: private methods with little or no dependence on data in the class they were defined in.

It struck me that these private methods didn't really belong to the classes they were in. Of course extracting them to new classes would mean making them public so there was no point to them being private in the first place.

Is this universal to all private methods or have I just not looked long enough to find private methods that look like they belong in the class their in?

Update:

Judging from the responses the general consensus on this site seems to be that they are not. I was curious if there were some who disagreed. I stumbled across a debate on c2.com that brings up some interesting counterpoints.

In any case it seems that by themselves private methods don't really indicate any particular opportunity for refactoring. Thanks for all the input.

Answer 1

No, private methods are often useful to reduce code duplication.

However, private methods that don't rely on object state are a code smell to me. If they exist, they should be marked static (if your language permits) to make your intentions clear.

Answer 2

Private methods allow you to break apart routines that normally get replicated throughout your code while still keeping them locked away safe inside your class. I don't see anything wrong with them at all.

Answer 3

Private methods are not code smell.

In fact, I think the opposite - A lack of private methods suggests, to me, that there is some good opportunity for refactoring in a class.

---

Edit:

One other thought for you...

If many (most?) of your private methods have no dependency on class state, you can make them static private methods. Many tools such as Resharper suggest this by default, since it makes it obvious that they have no dependency on the data in the class, and enforces that by the compiler.

This can help you understand which of the methods are touching your class and using data (which may, at some point, mean that there are more likely synchronization issues if you're multithreading, etc).

Answer 4

Private methods are a good thing: they let you break code up into bite-sized operations, without cluttering your classes' public interfaces with implementation details.

I would argue that public methods that aren't (or shouldn't be) called from other classes stink.

Answer 5

Private methods in themselves are not a code smell. Private methods with a lot of input parameters could be a code-smelly indication that they belong somewhere else.

If you're really interested in these topics, I can strongly recommend reading the "Clean Code" book. It's full of advices on how to (re)organize your code into small, self-descriptive methods with no more than 2-3 parameters. And by "small" the author means no more than 5-10 lines of code.

Answer 6

I find that having a number of small (in length) private methods which are "consolidated" as you say, in a public method, can make writing the code easier and easier for some one else to understand the code. This is good if others will look at the code or you might look at the code some time in the future.

I don't think it is fair to say that because something is a private method it is bad code.

But yes, if they don't seem to belong to that class, then get them out of there and put them in the class that they should logically belong to.

Answer 7

It very much depends on your situation but generally speaking, if it is something that can be used multiple times within the class and nothing outside the class needs it then it should be private. Also you can use private methods to break up lare methods into smaller more manageable pieces. Use the least privileged access possible for all your methods. This includes evaluating those that are public and asking yourself if they should be visible outside the assembly they are created in. Many times the answer is no.

Answer 8

private methods smell good to me. Methods that perform repetitive specific tasks that will never be extended/overwritten are perfect candidates for being private methods IMO

Answer 9

Private methods are not, in and of themselves, a code smell. It's not uncommon that multiple public methods will want sections that do the same thing as one another, and extracting that commonality into a private method makes sense.

However:

none of the private methods relied on any member variables.

That suggests that the private methods should be static - and a lot of private static methods suggests a utility class (in which those methods would be public) waiting to sprout. With respect to the original class's API, its interactions with those methods are still private - they don't pollute its namespace - but it takes advantage of the public (and therefore directly testable) interface of the new class.

Answer 10

Having private methods that don't reference anything would seem to be a concern to me, but, I agree with the earlier responses that these could be good private static methods.

You may want to look and see if these are being called from more than one method. If only one method calls them then you may want to refactor and replace that code in the method that was calling it.

Private methods are what you want, as you want as few ways into the class as possible, so that the class is more self-contained,

Answer 11

After reading Clean Code and especially the section where he explains that a in an ideal world the instance methods of a class should use all member variables I created a little plugin for Eclipse to help with this. The plugin lists all member variables in a tree and under each variable you can see the methods that uses it. There is also a reverse view so you can see which fields are used by which methods. This can also be used to easily see which methods could be static. It is mostly a proof of concept right now but feel free to try it out: link text

Drop that jar in your ecplise/plugin directory and open the view from Show Views->Other>Refactoring...

Then select something in the package hierarchy.