The Case Against Private Methods

Yeah. This post is titled that.

Please, consider the following

Suppose we have a class with an absurdly large method. (For convenience, I’m going to use comments.)

class Service : IService
{
    public void DoSomething()
    {
        // initialize the process

        // a chunk of code that's used in other classes

        // prep for the next step

        // a chuck of code that's used only in this class
        // but in multiple places

        // prep for the next step

        // a chuck of code that's used only here
        // but is still somewhat distinct

        // finish up
    }
}

This class is suffering from a dire need to be refactored. The reason we want to refactor this code is three-fold.

  • Readability
  • Eliminate code duplication
  • Testability

Let’s take each refactoring opportunity separately.

Code used in other classes

This one’s pretty easy actually, and fairly common. The steps we take are:

  1. Refactor the functionality into a new class.
  2. Abstract an interface.
  3. Inject the interface into this class (and any other classes which contained the duplicated code).
  4. Use the interface instead of the code chunk.

Our class how looks like this:

class Service : IService
{
    private IMultiuseFunctionality _multiuseFunctionality;

    public Service(IMultiuseFunctionality multiuseFunctionality)
    {
        _multiuseFunctionality = multiuseFunctionality;
    }

    public void DoSomething()
    {
        // initialize the process

        _multiuseFunctionality.DoIt();

        // prep for the next step

        // a chuck of code that's used only in this class
        // but in multiple places

        // prep for the next step

        // a chuck of code that's used only here
        // but is still somewhat distinct

        // finish up
    }
}

Good. Now that that’s done, let’s see how well we’ve accomplished our goals.

Readability

The code looks a bit better. We’ve isolated the code, which means less code to concentrate on. Because we’ve named the interface and its method appropriately, we know what that chunk of code does, and it makes the code around its usages easier to read, too.

Eliminate code duplication

The we’ve consolidated the logic into a single place. If we need to change it (for bug fixes or enhancements), we only have to modify one portion of the code.

Testability

Because the code is now located in an interface-implementing method, we can easily access it in a unit test.

Well done!

Code that’s used multiple times in only this class

We could use the exact same approach as before to achieve the same result, but this time we don’t think that it’s of any benefit to move the functionality into an entirely separate class since it’s not used anywhere else. A private method could handle this.

After that our class looks like this:

class Service : IService
{
    private IMultiuseFunctionality _multiuseFunctionality;

    public Service(IMultiuseFunctionality multiuseFunctionality)
    {
        _multiuseFunctionality = multiuseFunctionality;
    }

    public void DoSomething()
    {
        // initialize the process

        _multiuseFunctionality.DoIt();

        // prep for the next step

        _DoSomethingHere();

        // prep for the next step

        // a chuck of code that's used only here
        // but is still somewhat distinct

        // finish up
    }

    private void _DoSomethingHere()
    {
        // a chuck of code that's used only in this class
        // but in multiple places
    }
}

Not bad. Let’s see how our goals are doing.

Readability

The code still looks a bit better from before. We’ve still isolated the code, albeit locally. Because we’ve named the method appropriately, we know what that chunk of code does, and it makes the code around its usages easier to read, too.

Eliminate code duplication

The we’ve consolidated the logic into a single place. If we need to change it (for bug fixes or enhancements), we only have to modify one portion of the code.

Testability

Um… no. We didn’t improve the testability. In order to access the code we refactored, we still have to go through the main DoSomething() method.

Meh. The interface option is probably better, but we can live with this… I guess.

Code that only used in one place but still implements distinct functionality

This is probably the hardest one to see. It’s code that generally performs the heart of the logic that the main method is intended to do. But since it still is distinct functionality, it can be refactored into its own method. So we use the private method approach again.

Now our code looks like this:

class Service : IService
{
    private IMultiuseFunctionality _multiuseFunctionality;

    public Service(IMultiuseFunctionality multiuseFunctionality)
    {
        _multiuseFunctionality = multiuseFunctionality;
    }

    public void DoSomething()
    {
        // initialize the process

        _multiuseFunctionality.DoIt();

        // prep for the next step

        _DoSomethingHere();

        // prep for the next step

        _DoTheThing();

        // finish up
    }

    private void _DoSomethingHere()
    {
        // a chuck of code that's used only in this class
        // but in multiple places
    }

    private void _DoTheThing()
    {
        // a chuck of code that's used only here
        // but is still somewhat distinct
    }
}

Okay. Now our interface method seems relatively succinct. But this refactoring has the same problems as before: we can’t test the methods because they’re private. Ordinarily, we just deal with it.

The remedy

As I mentioned before, one solution would be to refactor into interfaces. But that seems like overtaking the plumbing.

I suggest continuing with the method refactor (as we did), except rather than making the methods private, they should be public!

“But wait,” you cry. “if you make them public, then anyone can call them!”

True… sort of.

Note that our class is implementing an interface, IService. This is because we’re building our application the right way, using dependency injection and an IoC container. That means that there should only be two references to the actual class: the IoC container and the unit test. Everything else should be interacting solely with interfaces. And since these public methods are not defined on any interfaces, you have to have a reference specifically typed to this class in order to call them.

We know we’re not going call any specific methods (interface or otherwise) from the IoC, and we’d like to be able to call these methods specifically from unit tests. So I think we’re good.

Now, our method is readable, we’ve eliminated any code duplication, and all of our chunks of code are independently testable. Yea!

Wrapping things up

Given that we want to be able to modularly test things, it really seems like we would want to test each chunk of code that we write independently. We can’t do that if we hide details in private methods.

NOTE There is an argument to be made that if we have a method with portions that we should test independently, then we should refactor it into separate interfaces because our class is doing to much, thereby violating SRP. However, on occasion, even when we adhere to SRP, we can still end up with a few large methods.

Understanding this, I can’t think of a terribly convincing reason to have a method be private.

The next post will be a bit shorter, but we’ll expand on this idea a bit, focusing on the IoC container.

Advertisements

8 thoughts on “The Case Against Private Methods

    1. That’s usually the case, and I think that way, too. But sometimes (and you know it’s true) the exposed method has a lot of steps. I feel safer when I can validate each of those steps is occurring properly.

      Like

      1. Yes, but I would respectfully reason that regardless of the steps involved within a method, there should be only one result if that methods still conforms to single responsibility. Thus, it’s the result/behavior that I would test for and not how that result was derived (aka: implementation detail).

        Like

  1. Why do you feel it important to create an interface for every class? An interface that only ever has one implementor is just noise. If you do this simply for testability, then ok I guess, but that slightly harms the readbility of the codebase.
    Religiously defining an interface for every class violates the DRY principle. It is needless repetition until you actually have two implementors.
    Is there some reason other than “this gives me an easy test seam” to automatically define an interface?

    Sure interfaces are preferred for APIs or re-usable components. But for every single class? It’s not wise, in my opinion. You can always extract the interface later when you actually need it.

    Like

    1. If you don’t extract an interface, then you’re violating SOLID, primarily the Dependency Inversion Principle, which states that we should depend upon abstractions (entities without any specific implementation). In .Net we use interfaces. You could use an abstract class, but there’s more of a chance you could have implementation details included.

      Also, your violating the Liskov Substitution Principle in that we can’t necessarily replace the dependency with something else. Interfaces allow that easily.

      I understand your argument that only having one implementor of an interface could violate DRY, but I don’t think it does since an interface is nothing more than a contract that states a class will provide a certain set of functionality. It’s not repeating the class details; the interface has a different purpose.

      In short, I think the benefits are sufficient to warrant creating an interface for any class that may be a dependency somewhere else. Exceptions to this may include a class that is resolved and invoked from within the entry point (e.g. Program.Main()).

      Like

      1. Thanks for the reply. You’re helping me think about this from other perspectives, and I appreciate your time.

        Sure creating all those interfaces increases flexibility, but flexibility has a cost.
        I have actually attempted to follow the “interface for every class” approach in the past. When you immediately produce an interface, whether strictly required or not, you end up with hundreds of tiny IFoo.cs files littering your codebase. I decided that it was not worth the reduced readability and the hassle (albeit minor) of creating all these unused interfaces.

        Maybe I didn’t understand how to fully benefit from those interfaces, but it really seems like a lot of cruft just to provide a test seam and/or feel good about my design because it’s “future proof”. And then on top of that, if you’re using a DI container, you have to wire up the implementation to the interface in some 3rd location.

        RE: DIP
        I don’t find that it violates the spirit of the “depend on abstractions” rule because the publicly visible methods are the “interface” to that class (in the general sense of the term “interface”). That is, you only interact with the object by passing messages, and there are a well defined set of messages. If and when you need to supply two different implementations that satisfy the same interface, you can then fall back onto the C# mechanism “interface” in order to enable callers to use either implementation. In a language that supports duck typing, this is an unnecessary step (whether that’s good or bad is another discussion).

        This all depends on what you believe qualifies as an “interface.” I consider the public methods to be the “interface” to the class. For you, clearly the methods don’t satisfy that definition (you even suggest making internal details public, which of course would be contrary to my approach).

        RE: LSP
        No LSP violation can occur because I’m referring to the scenario where there is only one implementation of an interface. If there were two implementations, then I absolutely agree with you. As soon as you find that you need two implementations, you must use some technical mechanism to achieve this, such as the “interface” keyword, an inherited class, or duck typing. Until that time arrives, apply YAGNI.

        RE: DRY
        The C# approach to “interface” is inherently not DRY because you have the same information captured in two places. If you add a method to the interface, you have to update the class, and vice-versa. It’s so redundant that we have have to rely on refactoring tools to change the source code in each spot that is impacted.
        In the general case, I don’t know how to eliminate this redundancy. I don’t claim to have the design answer. It seems necessary to duplicate the definition of the contract and the implementation of the contract due to decisions made by the language designers. However, you can avoid this redundancy in cases where an “interface” is not strictly required.

        Thanks again for sharing your views. If I have overlooked or undervalued something, please let me know.

        Like

  2. Greg, I think you are misunderstanding DRY, LSP and DIP principles.

    DRY is a more general principle that can be applied for anything, including programming. It means that we should not repeat anything, whether they are just method signatures or implementations. If we are creating an interface that just has a single implementation, we are repeating ourselves anyway. Obviouly the IDEs can do the repetitive work for us. However, if we have just one implementation, we can directly create a class and further extract an interface if we really need one. What you are doing is “previewing” future requirements that you may not even need. That goes against other general principle called YAGNI: https://en.wikipedia.org/wiki/You_aren't_gonna_need_it.

    About the LSP and DIP, they are not about interfaces in the actual meaning of the “interface” keyword existing in programming languages. It is about “interfaces” meaning “abstractions”. If you look at the LSP and SOLID articles in Wikipedia, and at the links to the Uncle Bob articles (that coined the term SOLID), you’ll see that the word “interface” doesn’t even appear at the definitions of these principles, but the word “abstraction”.

    LSP is all about polymorphim, thus considering you have a reference to a Vehicle class, if you change this reference to point to a Car or to a Airplane instance, the client code that relies only on the Vehicle public interface will continue to work and will not realize the difference. And when we say “public interface” it doesn’t mean that we are using actual interfaces. A class has a public interface that is defined by the public method that it declares or inherits.

    The only point is that, usually, dependency injection frameworks will just inject objects that were declared using an actual interface. By using a class instead of an interface doesn’t mean we are violating these principles. In the Vehicle example, I don’t have any interface. If I have a method that expects a Vehicle and I pass a Car or a Airplane, it will work as well. Thus, I’m able to change the Vehicle with any object from a subclass at runtime, without affecting how the program works (if the method really relies only on the public interface of the Vehicle class).

    Liked by 2 people

    1. No, I understand those principles. My point was just to say that if you’re depending upon an abstraction, you can’t access any additional public methods. Since that’s the case, it shouldn’t hurt anything (in fact it can help) to have implementation details exposed, if only for testability.

      This is in line with LSP and DIP.

      Certainly, it feels weird, and I don’t do it in my actual code (see any of my open source on github). It was just an exploration of the idea.

      Liked by 1 person

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s