Breaking public member functions into lots of private member functions

Question

When I write a class's public member function that does several things, like..

void Level::RunLogic(...);

In that function I find myself splitting it up into several private member functions. There's no point in splitting the public member function up into several functions because you wouldn't do one thing without the other, and I don't want the user worrying about what to in what order etc. Rather, the RunLogic() function would look something like this...

void Level::RunLogic(...) {
 DoFirstThing();
 DoSecondThing();
 DoThirdThing();
}

With the DoThing functions being private member functions. In Code Complete, Steve McConnel recommends to reduce the number of functions you have in a class, but I'd rather not just put all that code into the one function. My assumption of his true meaning is that a class shouldn't have too much functionality, but I'm just wondering what other programmers think regarding this.

In addition, I've been moving towards exposing less and less implementation details in my public member functions, and moving most of the work to small private member functions. Obviously this creates more functions...but that's where the question lies.

Answer 1

One rule that I use is the rule of three. If I am doing the same thing three times in different places, it's worth having a separate (possibly private) member function that encapsulates that shared behavior.

The only other rule that I generally use in this situation is that of readability. If I'm looking at a member function that fills more than a full screen in my IDE, I find it hard to trace all the code paths and possible wrong turns. In that case, it's perfectly reasonable to break one monolithic member function into two or more private helper functions.

Regarding the Code Complete comment about the number of functions in a class, remember that the greatest risk involved in the number of functions is primarily linked to the complexity of the externally available interface. The more entry points you provide for other people to work with, the greater the likelihood that something will go wrong (e.g., due to a bug in one of the public members, incorrect input, etc). Adding a private member function does not increase the complexity of the public API.

Answer 2

I recommend breaking them into separate methods, where each method takes care of one small task, and have a descriptive for each private method. Breaking the methods up and the method names would make the logic code more readable! Compare:

double average(double[] numbers) {
  double sum = 0;
  for (double n : numbers) {
    sum += n;
  }
  return sum / numbers.length;
}

to:

double sum(double[] numbers) {
  double sum = 0;
  for (double n : numbers) sum += n;
  return sum;
}

double average(double[] numbers) {
    return sum(numbers) / numbers.length;
}

Code Complete addresses the interface that each class exposes, not the implementation.

It may make more sense to make those smaller methods as package protected, so you can easily unit test them, instead being able to test the complicated RunLogic only.

Answer 3

You are unintentionally following the ff rules:

A class should have only one reason to change / Single responsibility principle

Which is a good thing. By properly separating the responsibilities you are making sure that your app won't easily break due to change

Answer 4

You are right to want to keep the public method simple, and split its functionality into multiple private methods.

McConnell is right that you should reduce the number of methods you keep in a class.

Those two goals are not really at odds. I don't think McConnell would advocate making your functions longer to reduce the number of them. Rather, you should consider pushing some of those private methods into one or more utility classes that can be used by the public class.

The best way to accomplish this will depend on the details of your code, of course.

Answer 5

I agree with splitting the functionality.

I've always been taught and have stuck to the knowledge that a function is defined to perform a single encapsulated task, if it seems to be doing more than one thing then it may be feasible to re-factor it. And then a class to encapsulate similar or related functions together.

Splitting these tasks down and still using a single public member in my opinion allows a class to perform that important task in the way it was intended, whilst making it easier to maintain. I also often find that there are multiple similar sections of code in the same complex method which can be re-factored into a single generic function with parameters - both improving readability and maintainability; and even reducing the amount of code.

Answer 6

I don't see the advantage of keeping especially public member functions as small as possible (measured in lines of code). Generally, long functions tend to be less readable, so in most cases, spreading the implementation of big functions improves readability, no matter whether they are part of a class or not.

As to the good advice of keeping classes as small as possible, this is especially important for the public part of the interface and for the amount of data encapsulated. The reason is that classes with lots of data members and public functions undo the advantages of data encapsulation.

Answer 7

A 'rule of thumb' that I use is that no one function will take more than a screen full space at a time. Although rather simple minded, it helps when you can 'take it all in' one screen at a time. It will also limit (by definition) the complexity of any one method. When you are expected to hand over your work to a co-worker for further maintenance, upgrades and bug fixes(!), keeping it simple is good. With that in mind, I agree that keeping your public methods simple is almost always the correct choice.