Appropriate access-level practices when breaking up large methods into smaller ones?

Question

So I am a relatively new programmer and am still learning, and have run into a bit of an issue (so apologies if my tags are a little off). Early on I had a chance to read an article discussing the advantages of breaking up methods that contained lots of code into sets of calls to many smaller, clearly-named methods. In general I felt like this made cleaner looking code, and definitely made unit testing a lot easier. However, I have some concerns about whether to make all these new methods public or private. Making them private seems like the right thing to do, since the rest of the code generally doesn't need access to these methods. However, unit testing private methods can be messy. Is there a best practice for this?

What I'm doing right now:

 public class WashingMachine {
      public Load wash(Load load) {
           // Removes one sock from load
           for (ClothingItem item : load.getItems()) {
                if (item.getType().equalsIgnoreCase("sock") {
                     load.removeItem(item);
                     .. // logic for sending sock to proper dimension
                     break;
                }
           }
           // rest of logic for cleaning clothes
      }
 }

Turns into:

 public class WashingMachine {
      // Wash a load of laundry
      public Load wash(Load load) {
           // Removes one sock from load
           removeSock(load.getItems());
           // rest of logic for cleaning clothes
           ..
      }

      // Oh no, I can't unit test this easily!
      private void removeSock(List<ClothingItem> items) {
           ...
      }
 }

have a look at this question [link](http://stackoverflow.com/questions/34571/whats-the-best-way-of-unit-testing-private-methods) — Stochast, Jul 19 '11 at 20:08

Reverend Gonzo · Accepted Answer · 2011-07-19T20:31:14.940

Think of your public methods as functionality you want to allow anybody to use.

Some of this functionality may be complicated, and you're right, that functionality should be split up into different methods. The question is, "What kind of access do they have?"

Well, if they're public, now external classes can access your internal functionality, which you really don't want. Even if it won't break anything, when you start working on large projects (with larger teams of people), by making it public, other programmers might use that functionality, which now prevents you from changing it.

For example, taking a car analogy. Say your method is accelerate(). This will probably call releaseGas(). However, you don't want anybody externally to release gas. It should only be called in a controlled environment.

Then there's protected access. Protected access is for methods that may be overridden by extending classes. These should be methods that do a specific chunk of work related to internal functionality. Taking the car example again, there might be a RaceCar clas which uses a special type of gas, and so it wants to provide its own method of releasing gas. This would be a reason for releaseGas to be protected.

As for tests, you should be testing, primarily, your public contract. That's what other classes use, and at the end, that's what really matters. Your internal methods are internal to your functionality, and will be tested by way of testing your public contract. This also means your class should be designed based on its external uses first, and the tests built around that. (Even with test-driven development, this gets easier as you gain experience.)

Granted, sometimes those internal methods are complicated enough that they warrant their own unit tests. However, you don't need to make them protected. In that case, you can make them default access. (Neither public or protected). As long as your unit test is in the same package, you'll be able to call them.

Better yet, if you know that you don't want anyone to extend it but you need it to be tested, make it protected final or just final. This still allows being able to call it, but at least prevents it being overridden.

Edit:

Ryan's comment below is dead on. If your class is getting complex enough that it needs testing of its internal methods, those should probably be extracted into their own class, which can then be independently unit-tested.

Overall, your tests should be testing the public contract of your separate units.

+1 because I agree at every point except one: don't make "internal" methods package scope for testing purposes. If the code is complex enough to need its own set of tests, the obvious implication is that it should be its own unit. Extract it into a separate class/object with its own responsibilities. I used to do exactly what you recommended, and I've found it leads nowhere but into problems. Always let the tests guide you. — Ryan Stewart, Jul 19 '11 at 20:22
I will second this comment. If it's getting that complex, that code should probably be extracted into its own class which can then have its own unit test. — Reverend Gonzo, Jul 19 '11 at 20:26

score 1 · Answer 2 · answered Jul 19 '11 at 20:09

I think you should design your class based on the (main, not test) code that will be calling it. If no one has a need to ever call WashingMachine.removeSock() then I would leave it private.

As for the unit test, sure this means you can't test this one individual piece of logic within WashingMachine - but you can still test the wash() method, which if this is all that the other components using WashingMachine have visible, is all you really need to concern your test with.

It is possible to write a unit test that is too granular and concerned with details.

Appropriate access-level practices when breaking up large methods into smaller ones?

2 Answers2