53

I've found myself that I tend not to have private class functions. If possible, all candidates to private class function rather I put in to unnamed namespace and pass all necessary information as function parameters. I don't have a sound explanation why I'm doing that but at least it looks more naturally to me. As a consequence I need to expose less internal details in the header file.

What is your opinion - is it correct practice?

Wolf
  • 9,679
  • 7
  • 62
  • 108
drumsta
  • 1,336
  • 2
  • 16
  • 27
  • 2
    I think it's a really tough question (+1). And I think there are still some valid answers open... – Wolf Sep 02 '14 at 09:38

4 Answers4

24

In the semi large projects where I usually work (more than 2 million lines of code) I would ban private class functions if I could. The reason being that a private class function is private but yet it's visible in the header file. This means if I change the signature (or the comment) in anyway I'm rewarded sometimes with a full recompile which costs several minutes (or hours depending on the project).

Just say no to that and hide what's private in the cpp file.

If I would start fresh on a large c++ project I would enforce PIMPL Idiom: http://c2.com/cgi/wiki?PimplIdiom to move even more private details into the cpp file.

Just another metaprogrammer
  • 11,499
  • 2
  • 35
  • 54
  • 2
    It's not a 100% rule. Anonymous namespace has a massive drawback of being unable to write UTs for functions in it. Full recompile can be a negligible price to pay in comparison. Personally, I'd just ban anonymous namespaces and enforce UT coverage for private class functions, at least for not-so-stable projects. – sankalpn Oct 28 '16 at 19:14
  • 3
    I wasn't sure so had a Google.. "UT" means Unit Test? – Xenial Jul 08 '18 at 08:52
  • 1
    You must have a responsibility mess if whole program is being recompiled after a change in a single class. – mip May 09 '21 at 18:16
  • 2
    @sankalpn are you writing unit tests for private functions? I don't think that is recommended. Unit tests should test the public interface of a class. If you find that the class is complex enough that you want to test private functions, then you should try to extract those private functions into a new helper class. Then you can test the public interface of the helper class and just have the original class delegate some responsibility to the helper class. If you do this well you will often discover that the helper class can be reused in other parts of the codebase. – Eric Roller Oct 02 '21 at 15:19
11

I've done this in the past, and it has always ended badly. You cannot pass class objects to the functions, as they need to access the private members, presumably by reference (or you end up with convoluted parameter lists) so you cannot call public class methods. And you can't call virtual functions, for the same reason. I strongly believe (based on experience) that this is A Bad Idea.

Bottom line: This sounds like the kind of idea that might work where the implementation "module" has some special access to the class, but this is not the case in C++.

  • You're correct that sometimes parameter list is large enough. That's definitely drawback of this approach. On the other hand, I'm trying to stay away from use of any virtual function, so I very rarely fall in to need to call virtual function. Anyway thanks for the answer! – drumsta Aug 12 '10 at 21:20
  • 2
    @kriau Why stay away from the use of virtual functions? And you have not removed your dependency on the header. It strikes me that what you may be looking for here is the PIMPL idiom. –  Aug 12 '10 at 21:23
  • +1 for Pimpl (even though it's not part of the original answer) – Matthieu M. Aug 13 '10 at 06:53
  • 5
    Like any technique, it's easy to overdo it. Nevertheless, I've seen a ton of classes with private methods that are really just helper functions. Moving these out of the headers has a lot of benefits and doesn't suffer from the gotchas you've described. – Peter Ruderman Aug 13 '10 at 14:05
  • 2
    You don't necessarily _need_ access to private members of a class. If you need access, make a private method. Otherwise, why unnecessarily couple that method to the internal state of the object? – weberc2 Jun 28 '13 at 14:57
  • I don't think it is useful to point at a single use case and reject the entire concept. How about I want to improve the readability of my code by replacing my if(blah && !stuff || (thing > 10)) tests with bool ShouldDoSomethingNow() checks? – Liam Sep 01 '17 at 10:31
  • 1
    I use PIMPL only when I need it (for instance to avoid include conflicts in headers). – mfnx Sep 13 '18 at 15:01
7

It basically comes down to a question of whether the function in question really makes sense as part of the class. If your only intent is to keep details of the class out of the header, I'd consider using the pimpl idiom instead.

Jerry Coffin
  • 476,176
  • 80
  • 629
  • 1,111
3

I think this is a good practice. It often has the benefit of hiding auxiallary structures and data types as well, which reduces the frequency and size of rebuilds. It also makes the functions easier to split out into another module if it turns out that they're useful elsewhere.

Peter Ruderman
  • 12,241
  • 1
  • 36
  • 58