Private Methods are a Code Smell
kent.spillner.org
kent.spillner.org
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.
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. Moving these methods to appropriate collaborators (again, creating new classes as necessary) exposes such dependencies. Eliminating these dependencies simplifies the new API, which improves readability and understanding.
Here he seems to be complaining about bugs that arise due to overly complex mutable state. However that has nothing to do with private methods. The defensible assertion here might be that if you have a big ugly method that has too many state dependencies, then refactoring it into private methods isn't really an improvement even if it appears more readable. However that's a string of failures that only incidentally have anything to do with private methods. It would be more accurate to just state that non-functional code is a smell, increasing in putrescence as a function of the quantity of moving parts.
The resulting code is simpler, more testable, more reusable, more cohesive, and less coupled.
Again, the correlation of privateness to these concerns is dubious. Separation of concerns should be on a logical basis, not based on the structure of the implementation. Every additional class adds mental overhead. Decoupling is useful as a means of reusing code, but only if there is a case for reuse. It's also useful as a way to tease apart complex dependency graphs, but only if the resultant sets of dependencies make sense on their own. Also, let's not forget that encapsulating concerns in separate objects is no more functional than a bunch of private state if you end up passing around objects anyway... Law of Demeter and all that.
What I suspect here is that the author saw some bad code that used a lot of private methods and then honed in on that as a smell even though it has little general applicability. The bar needs to be much higher for declaring any core language construct to be a smell.
I don't like having state spread everwhere, but I hate being lied to about it.
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.
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.
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.
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
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.
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.
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.
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.)
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.
"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.
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!
"The second is that smells don't always indicate a problem. Some long methods are just fine. You have to look deeper to see if there is an underlying problem there - smells aren't inherently bad on their own - they are often an indicator of a problem rather than the problem themselves."
http://martinfowler.com/bliki/CodeSmell.html
Granted, you don't have to agree with Martin Fowler. I'm just making the case that it's not unreasonable for me (or the article's author) to define code smells that way.
Stating something is a smell doesn't make it so.