Private methods are a code smell

Original post:

www.koffeinfrei.org/2019/02/09/private-methods-are-a-code-smell

Just to get it out of the way up front: I'm not saying you should never use private methods.

I'm trying to convey something a bit more nuanced. If you aren't a person that can appreciate the grey zones of life this blog post may not be for you. Or maybe even so. You should decide for yourself. If you can't do that, then this blog post may not be for you. Or maybe it is nevertheless… Let's do this.

Your private is someone else's public

The bottom line is that a private method of one class could be a public method of another class. Every functionality is a public interface of something.

If it has to be made private this usually means that it is in the wrong place, i.e. in the wrong class.

Don't violate the SRP

Making a method private often means this: "This method doesn't really belong in this class, so I'll just make it private so nobody notices." It's just sweeping the dirt under the carpet. Just clean your floor properly and move this method out of that class.

Private methods should not be tested

I believe that private methods should not be tested, even if your language supports calling them. Tests should only verify the public interface of a class. A private method is an internal implementation detail which may change and thus will break the test.

If the functionality of a private method is trivial it may be fine not to explicitly test it. If it is non trivial though, testing the behavior of a class’s private methods indirectly through the class’s public methods is pretty much the same as testing it directly. If the private method has a complexity that should be under test, it should be public. If it doesn't make sense to be public in that class, it should be moved to another class.

If your goal is to achieve high test coverage, everything should be public and therefore testable.

Public interfaces should only expose what's relevant

The consumer of a public interface should not be bothered with unnecessary internals. A method that does not seem to belong to the public interface usually does not have any business of being part of that class and should usually be moved somewhere else.

Did you say reusability?

Having private methods excludes that functionality from being reused. If that functionality is moved to a public method though it can be consumed by someone else as well.

Internal helper methods are (probably) fine

There are cases where a private method is fine. If a method is really just a helper for a public method and doesn't make sense to live on its own in another context then it's fine to keep it as a private method. If you start to touch any of the guidelines mentioned before, this private method may evolve though from being private.

An example

This is just a really trivial academic example, but it should be enough to illustrate my point:

class Car
  def start
    ignite
  end

  private

  # Is ignition the responsibility of the `Car`?
  # Or rather of the motor?
  def ignite
  end
end

Let's move the private method to the Motor class:

class Car
  attr_accessor :motor

  def start
    motor.ignite
  end
end

class Motor
  def ignite
  end
end

The "private" story here is this:

The car starts by igniting itself

or

The car starts by igniting the motor itself

The refactored story on the other hand is this:

The car starts by telling the motor to ignite

4 questions to get private methods out of your face

If the answer to any of the following questions is "yes" you should consider extracting the private method:

• Does this method violate the SRP?
• Should I test this method?
• Is this method relevant as part of the public interface?
• Could this method be reused and be consumed by another class as well?

Perfection is never achieved

Having no private methods at all is the result of a perfect software design. This doesn't imply that we cannot use private methods at all. It means that we should try and strive towards a perfect design and reduce the use of private to a minimum.

Comments

[Jesse Phillips]

Why isn't the motor a private/module/package class? I ask because I not sure adding an additional layer to eliminate a private class really helps in readability or function. But to your main point, it should be considered.

[Alexis Reigel]

Why isn't the motor a private/module/package class?

Having private classes is the same as having private methods. It hides information that could be public. A Motor is just as a valid and standalone class as a Car is. Why should we hide functionality from being reused?

I not sure adding an additional layer to eliminate a private class really helps in readability or function

That's exactly my point. You'll end up with more classes in the end, each of which is more concise and properly adhering to the SRP than the class with private stuff in it. Having more specialized small parts is usually better than having a smaller number with less clear boundaries.

[Jesse Phillips]

Adhering to the SRP just to adhere to it isn't helpful in writing "better" code.

I'm not a huge fan of the access modifiers, I generally find that I want access to most things at some point. But that is still different than a new class needs to be created so we can jump through additional objects to reach the programs logic.

[Alexis Reigel]

Adhering to the SRP just to adhere to it isn't helpful in writing "better" code.

The SRP is in my opinion the single most important guideline in writing clean code. I've never seen code getting worse while adhering to the SRP.

I generally find that I want access to most things at some point.

Exactly, thus everything should be as public as possible.

But that is still different than a new class needs to be created so we can jump through additional objects to reach the programs logic.

If you want to have every piece of functionality publicly available you won't get around to extracting to multiple smaller pieces at some point. When a software system grows you have to split up. And having to search for logic in many specific classes is easier than having to look for it in hidden private methods that logically don't belong where they are.

[Jesse Phillips]

And having to search for logic in many specific classes is easier than having to look for it in hidden private methods that logically don't belong where they are.

As nice as that sounds, private methods don't actually hide logic, I just don't see myself doing a search in the way you're describing it happens.

It's weird because I agree and disagree at the same time. I generally don't think class are required for most logic, they should generally be free functions which operate on the class/data. This way your class keeps its single responsibility and you have functions with one responsibility.

[Alexis Reigel]

private methods don't actually hide logic.

They usually do though, which is (in my experience) the main reason for them to be private (which this post details).

I generally don't think class are required for most logic, they should generally be free functions which operate on the class/data. This way your class keeps its single responsibility and you have functions with one responsibility.

Neither do I, it heavily depends on the language you're using. A lot of languages don't require you to use classes to encapsulate logic. Using classes actually enables the use of private logic. Using functional approaches leads to treating each function as a valid public entity, generally leading to less running into the "private code smell".