
Private Methods are a Code Smell - fogus
http://kent.spillner.org/blog/work/2009/11/12/private-methods-stink.html
======
Quarrelsome
I think this is possibly the most dangerous sweeping generalisation that I've
ever read here.

I've found throughout my career that the opposite is true. The more
encapsulation the more solid the system. Making the world public encourages
unnecessary dependencies.

~~~
blasdel
Encapsulation doesn't make this problem go away at all -- it just replaces
interface proliferation with secretive state.

I don't like having state spread everwhere, but I _hate_ being lied to about
it.

~~~
joe_the_user
Encapsulation _can_ an excellent step in making a problem go away.

First a mistake is made all over your code. Then it's made only in the private
method. Then it can be changed easily -- and reverted easily if the change
doesn't work.

Of course, this isn't always a good idea. But there are many times that it can
be a good idea.

------
viggity
I disagree, I'm a big fan of encapsulation/information hiding. I'd rather have
this stuff hidden 20-30 classes than have 200 new classes cluttering my
intellisense

~~~
joe_the_user
Now, I think he is talking about increasing the encapsulation through
collaborator classes so he's not against encapsulation.

I still agree that there aren't any inherent problems with the several-
private-methods-level of data abstraction. Rather, a program's level of data
abstraction should follow the needs of the application as a whole.

~~~
mquander
Wait a minute. How would using a collaborator class as a "functor" for your
formerly private methods _improve_ encapsulation? At worst, if the
collaborator were public, now other people with no use for your methods would
be able to use them; at best, if it were a "friend" class or the equivalent,
you would be exactly where you started, and only the caller would be using the
method.

~~~
joe_the_user
I think the approach the author is talking about is creating an _internal
class_ (either private or public).

It's common approach for large libraries. Each STL container has a public
iterator class contained in it and rails is filled with internal, private
classes that implement the public functionality.

That said, I don't think such constructs necessarily make code more
understandable. Unless your project is really large scale, they're more like
an exercise in showing the language and design constructs you've learned.

------
mrcharles
In fact, we can take this a step further, and remove classes altogether, and
just have everything be stand-alone global functions! Then anyone can use or
reuse them and before you know it, programs will all be amazing!

~~~
jluxenberg
This is essentially the approach taken by functional languages (they also
include ways to collect functions into logical units). Object-oriented
programming is all about mutating the state of an object with private methods,
though, so I think the author is trying to make his OO programming style look
/ feel more functional.

If you're still convinced that OO and mutable state are a good idea, read this
essay by the author of Clojure, it might change your mind:
<http://clojure.org/state>

------
clavalle
This approach would seem to be asking for an explosion in classes...it might
be useful if there were repetition among your private methods in different
classes but as a blanket code smell statement--that may be taking it a little
far.

------
joe_the_user
_Taking small steps to improve design leads to flashes of brilliant design
inspiration._

I would take issue with this statement. I've found that making small changes
can clutter a design. Instead of just changing a bunch of small things,
working out a crisp, simple overall architecture seems to work best.

Indeed, lately I'm worrying that having a good test suite lets me write code
that works but which I don't understand as well.

------
stcredzero
_Private helper methods indicate classes are doing too many things._

Absolutely wrong. Sometimes you do want to hide details. I can imagine many
domains where you'd need a priority heap, and where the rest of the object
model would _care not one bit_ what goes on inside such a beast. Exposing such
details is not useful. Paying for the instantiation of another cluster of
little objects to do so wouldn't be helping matters one bit.

------
martinp
"Often, such private methods have gnarly dependencies because they directly
access or modify internal state. Moving these methods to appropriate
collaborators (again, creating new classes as necessary) exposes such
dependencies."

This is usually why I create private methods in the first place. The methods
modify internal state and they're too specific to the problem in question to
move them into a separate class. Making them public just doesn't make sense
and seems to break the principle of encapsulation.

Also, wouldn't moving these methods into a new class affect performance? For
example if you have a private method that does some complex operations on a
private/local collection, instead of performing the operations directly on the
collection you have to pass the it back and forth to the public method in
another class.

------
jxcole
I used to be very much against private methods and though I have eased up a
little, I still prefer classes with only a small number of public methods and
no private methods.

It's true that you have a lot more classes, but these classes can be separated
into packages. The encapsulation is still there (private members vs. private
methods), and the encapsulated code has a greater ability to be reused or unit
tested (if you're into that.)

------
pmichaud
This is interesting, but I'm not sure how practical it is. Has anyone ever
tried this approach successfully?

~~~
j_baker
Bear something in mind: a code smell isn't something that's bad. It's
something that indicates that something _may_ be bad. Therefore, don't run out
and refactor all your classes with private methods.

However, if there is a class that has a lot of methods that you're not
comfortable exposing to the outside world, then there's a good chance that
there's a way to split that class and make your code easier to maintain.

Either that or you're being too picky with what methods the outside world can
access.

~~~
revolvingcur
I feel that the connotation of "code smell" is a little stronger than what you
state, but mostly I take umbrage to this statement:

"Moving these methods to collaborators and making them public creates
opportunities for future reuse without reducing the clarity of the original
code."

This is the crux of the argument. After all, if you aren't planning for reuse,
there's very little motivation to create a collaborating class unless the main
class really is getting excessively large. But if you're guessing at future
refactorings, especially when dealing with the types of specialized methods
being discussed, you're probably optimizing prematurely.

I do strongly agree that private methods often have unnecessary dependencies.
But this is a different design issue.

~~~
stcredzero
"Moving these methods to collaborators and making them public creates
opportunities for future reuse without reducing the clarity of the original
code."

That's not even always true. Sometimes, the best way to express an algorithm
may be with a recursive function. Making those separate objects might even
_slightly reduce_ the code's clarity. The code smell here should not be the
private methods. It should be such private methods with similar functions
repeated in several objects. Let "a need for reuse" become self evident,
_then_ refactor for reuse. Otherwise, you are just pre-optimizing. YAGNI!

