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.
Agreed. The whole idea is coming straight out of left field:
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.
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.
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
I agree with your sentiment. Further, new classes have a small (but not immeasurable) hit to performance as each object, when created, must be initialized and tracked in memory.
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.
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.
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.
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!
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
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.
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.
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.
"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.
The point of the posting wasn't to say "don't use private methods anymore". It's saying "private methods are an indicator that something might be wrong", which I agree with.
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.)
Yeah, I'd like to see an example of this showing where the creation of the private method to begin with seemed like a good idea, this refactoring technique, and how doing so improved anything.
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.
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.
"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!
"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."
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.
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.