When does the "Do One Thing" paradigm become harmful?

Question

For the sake of argument here's a sample function that prints contents of a given file line-by-line.

Version 1:

void printFile(const string & filePath) {
  fstream file(filePath, ios::in);
  string line;
  while (std::getline(file, line)) {
    cout << line << endl;
  }
}

I know it is recommended that functions do one thing at one level of abstraction. To me, though code above does pretty much one thing and is fairly atomic.

Some books (such as Robert C. Martin's Clean Code) seem to suggest breaking the above code into separate functions.

Version 2:

void printFile(const string & filePath) {
  fstream file(filePath, ios::in);
  printLines(file);
}

void printLines(fstream & file) {
  string line;
  while (std::getline(file, line)) {
    printLine(line);
  }
}

void printLine(const string & line) {
  cout << line << endl;
}

I understand what they want to achieve (open file / read lines / print line), but isn't it a bit of overkill?

The original version is simple and in some sense already does one thing - prints a file.

The second version will lead to a large number of really small functions which may be far less legible than the first version.

Wouldn't it be, in this case, better to have the code at one place?

At which point does the "Do One Thing" paradigm become harmful?

Answer 1

Of course, this just begs the question "What is one thing?" Is reading a line one thing and writing a line another one? Or is copying a line from one stream to the other to be considered one thing? Or copying a file?

There is no hard, objective answer to that. It's up to you. You can decide. You have to decide. The "do one thing" paradigm's main goal is probably to produce code that's as easy to understand as possible, so you can use that as a guideline. Unfortunately, this isn't objectively measurable either, so have to rely on your gut feeling and the "WTF?" count in code review.

IMO a function consisting of only a single line of code is rarely ever worth the trouble. Your printLine() has no advantage over using std::cout << line << '\n'¹ directly. If I see printLine(), I have to either assume it does what its name says, or look it up and check. If I see std::cout << line << '\n', I know immediately what it does, because this is the canonical way of outputting the content of a string as a line to std::cout.

However, another important goal of the paradigm is to allow code reuse, and that's much more objective a measure. For example, in your 2nd version printLines() could easily be written so that it is an universally useful algorithm that copies lines from one stream to another:

void copyLines(std::istream& is, std::ostream& os)
{
  std::string line;
  while( std::getline(is, line) );
    os << line << '\n';
  }
}

Such an algorithm could reused in other contexts as well.

You can then put everything specific to this one use case into a function which calls this generic algorithm:

void printFile(const std::string& filePath) {
  std::ifstream file(filePath.c_str());
  printLines(file, std::cout);
}

---

^{1 _{Note that I used '\n' rather than std::endl. '\n' should be the default choice for outputting a newline, std::endl is the odd case.}}

Answer 2

Having a function do only "one thing" is a means to two desirable ends, not a commandment from God:

1. If your function only does "one thing", it will help you avoid code duplication and API bloat because you'll be able to compose functions to get more complex things done instead of having a combinatorial explosion of higher-level, less-composable functions.

2. Having functions only do "one thing" may make the code more readable. This depends on whether you gain more clarity and ease of reasoning by decoupling things than you lose to the verbosity, indirection and conceptual overhead of the constructs that allow you to decouple things.

Therefore, "one thing" is unavoidably subjective and depends on what level of abstraction is relevant to your program. If printLines is thought of as a single, fundamental operation and the only way of printing lines that you care about or foresee caring about, then for your purposes printLines only does one thing. Unless you find the second version more readable (I don't) the first version is fine.

If you start needing more control over lower levels of abstraction and ending up with subtle duplication and combinatorial explosion (i.e. a printLines for filenames and a completely separate printLines for fstream objects, a printLines for console and a printLines for files) then printLines is doing more than one thing at the level of abstraction you care about.

Answer 3

At this scale, it doesn't matter. The single-function implementation is perfectly obvious and understandable. However, adding just a bit more complexity makes it very attractive to split the iteration from the action. For example, suppose you needed to print lines from a set of files specified by a pattern like "*.txt". Then I would separate the iteration from the action:

printLines(FileSet files) {
   files.each({ 
       file -> file.eachLine({ 
           line -> printLine(line); 
       })
   })
}

Now the file iteration can be separately tested.

I split functions to either simplify testing, or to improve readability. If the action performed on each line of data were complex enough to warrant a comment, then I would certainly split it into a separate function.

Answer 4

Extract methods when you feel the need for a comment to explain things.

Write methods that either only do what their name says in an obvious way, or tell a story by calling cleverly named methods.

Answer 5

Even in your simple case, you are missing details that the Single Responsibility Principle would help you manage better. For example, what happens when something goes wrong with opening the file. Adding in exception handling to harden against file access edge cases would add 7-10 lines of code to your function.

After you've opened the file, you're still not safe. It could be yanked from you (especially if it's a file on a network), you could run out of memory, again a number of edge cases can happen that you want to harden against and will bloat your monolithic function.

The one-liner, printline seems innocuous enough. But as new functionality gets added to the file printer (parsing and formatting text, rendering to different kinds of displays, etc.) it will grow and you'll thank yourself later.

The goal of SRP is to allow you to think about a single task at a time. It's like breaking a big block of text into multiple paragraphs so that the reader can comprehend the point you are trying to get across. It takes a little more time to write code that adheres to these principles. But in doing so we make it easier to read that code. Think of how happy your future self will be when he has to track down a bug in the code and finds it neatly partitioned.

Answer 6

I personally prefer the latter approach, because it saves you work in the future and forces "how to do it in a generic way" mindset. Despite that in your case Version 1 is better than Version 2 - just because problems solved by Version 2 are too trivial and fstream-specific. I think it should be done the following way (including bug fix proposed by Nawaz):

Generic utility functions:

void printLine(ostream& output, const string & line) { 
    output << line << endl; 
} 

void printLines(istream& input, ostream& output) { 
    string line; 
    while (getline(input, line)) {
        printLine(output, line); 
    } 
} 

Domain-specific function:

void printFile(const string & filePath, ostream& output = std::cout) { 
    fstream file(filePath, ios::in); 
    printLines(file, output); 
} 

Now printLines and printLine can work not only with fstream, but with any stream.

Answer 7

Every paradigm, (not just necessarily the one you cited) to be followed require some discipline, and thus -reducing the "freedom of speech"- result in an initial overhead (at least just because you have to learn it!). In this sense every paradigm can become harmful when the cost of that overhead is not overcompensated by the advantage the paradigm is designed to keep with itself.

The true answer to the question, thus, requires a good capability to "foreseen" the future, like:

• I am right now required to do A and B
• What is the probability, in a near future I will required to do also A- and B+ (i.e. something that looks like A and B, but just a bit different)?
• What is the probability in a more far future, that A+ will become A* or A*- ?

If that probability is relatively hight, it will be a good chance if -while thinking about A and B- I also think about their possible variants, thus to isolate the common parts so that I will be able to reuse them.

If that probability is very low (whatever variant around A is essentially nothing more than A itself), study how to decompose A further will most likely result in wasted time.

Just as an example, let me tell you this real story:

During my past life as a teacher, I discovered that -on the most of student's projects- practically all of them provide their own function to calculate the length of a C string.

After some investigation I discovered that, being a frequent problem, all the student come to the idea to use a function for that. After told them that there is a library function for that (strlen), many of them answered that since the problem was so simple and trivial, it was more effective to them write their own function (2 lines of code) than seeking the C library manual (it was 1984, forgot the WEB and google!) in strict alphabetic order to see if there was a ready function for that.

This is an example where also the paradigm "don't reinvent the wheel" can become harmful, without an effective wheel catalog!

Answer 8

Your example is fine to be used in a throw-away tool that is needed yesterday to do some specific task. Or as an administration tool that is directly controlled by an administrator. Now make it robust to be suitable for your customers.

Add proper error/exception handling with meaningful messages. Maybe you need parameter verification, including decisions that must be made, e.g. how to handle not existing files. Add logging functionality, maybe with different levels like info and debugging. Add comments so that your team colleagues know what's going on there. Add all the parts that are usually omitted for brevity and left as an exercise for the reader when giving code examples. Don't forget your unit tests.

Your nice and quite linear little function suddenly ends in a complex mess that begs to be splitted into separate functions.

Answer 9

IMO it becomes harmful when it goes that far that a function hardly does anything at all but delegate work to another function, because that's a sign that it is no longer an abstraction of anything and the mindset that leads to such functions is always in danger of doing worse things...

From the original post

void printLine(const string & line) {
  cout << line << endl;
}

If you are pedantic enough, you might notice that printLine still does two things: writing the line to cout and adding a "end line" character. Some people might want to handle that by creating new functions:

void printLine(const string & line) {
  reallyPrintLine(line);
  addEndLine();
}

void reallyPrintLine(const string & line) {
  cout << line;
}

void addEndLine() {
  cout << endl;
}

Oh no, now we have made the problem even worse! Now it's even OBVIOUS that printLine does TWO things!!!1! It doesn't make much stupidity to create the most absurd "work-arounds" one can imagine just to get rid of that inevitable problem that printing a line consists of printing the line itself and adding an end-of-line character.

void printLine(const string & line) {
  for (int i=0; i<2; i++)
    reallyPrintLine(line, i);
}

void reallyPrintLine(const string & line, int action) {
  cout << (action==0?line:endl);
}

Answer 10

Short answer... it depends.

Think about this: what if, in the future, you won't want to print only to standard output, but to a file.

I know what YAGNI is, but I'm just saying there might be cases where some implementations are known to be needed, but postponed. So maybe the architect or whatever knows that function needs to be able to also print to a file, but doesn't want to do the implementation right now. So he creates this extra function so, in the future, you only need to change the output in one place. Makes sense?

If you however are certain you only need output in the console, it doesn't really make much sense. Writing a "wrapper" over cout << seems useless.

Answer 11

The whole reason that there are books devoting chapters to the virtues of "do one thing" is that there are still developers out there that write functions that are 4 pages long and nest conditionals 6 levels. If your code is simple and clear you've done it right.

Answer 12

As other posters have commented, doing one thing is a matter of scale.

I would also suggest that the One Thing idea is to stop people coding by side effect. This is exemplified by sequential coupling where methods have to be called in a particular order to get the 'right' result.