Is it a good practise to put non member function inside a class?

Question

I'm reading a code like this:

class Member
{
public:
    friend std::istream& operator>>(std::istream& in, Member& m)
    {
        in >> m.name >> m.bYear >> m.bMonth;
        return in;
    }

    friend std::ostream& operator<<(std::ostream& out,const Member& m)
    {
        out << m.name << " " << m.bYear << "." << m.bMonth;
        return out;
    }

private:
    std::string name;
    int year;
    int month;
};

I never saw this way before. Is it a good practice to define nonmember function inside a class body with friend? Any pros and cons?

This is something I often do myself (for trivial, inline I/O) because it expresses the intimate connection the functions have with the class. I don't see any real negatives doing it this way. You have to *declare* the functions within the class definition regardless. — Galik, Oct 23 '14 at 08:33
Static functions are also non-member. If you want to put friends out of class you should put these too. I don't like to cut the connection between class and related functions otherwise that way it'd make it hard to track and hard to read. I don't see any down side really. — MahanGM, Oct 23 '14 at 08:36
If the class and its friend are templates, then it gets rather messy trying to define the friend outside the class. http://stackoverflow.com/questions/16814037#16814372 — Mike Seymour, Oct 23 '14 at 08:36
@MikeSeymour Not at all. Most of the places where I've worked have had the rule that the implementation code must be in a separate file, even for templates. The only difference is that with templates, the implementation file must be included by the header. — James Kanze, Oct 23 '14 at 08:43
@Theolodis: Not a duplicate, the use of `friend` is not under discussion. — Deduplicator, Oct 23 '14 at 08:46
@JamesKanze: Maybe we have different definitions of "messy". Having to declare the function template before the class and (in most cases) also declare the class template before the function certainly seems messy to me. — Mike Seymour, Oct 23 '14 at 08:47
@MikeSeymour Not sure I follow you. Having the interface specification lost in a lot of implementation code is messy. But where does the function template come into play? (If you have two classes, each of which accesses members of the other in the implementation, then you have to put the implementations after the definition of the two classes, even if they are templates. Otherwise, the language allows both, but in general, it's good practice to keep as much of the implementation code hidden as possible.) — James Kanze, Oct 23 '14 at 08:55
@JamesKanze: My (somewhat tangential) comment was that **if** they are templates, **then** it gets messy due to the extra declarations one needs, as demonstrated by the question I linked to. If they aren't templates, then you're right, it's no more messy (and perhaps preferable, according to personal taste) to separate the interface from the implementation. Sorry if I caused confusion by commenting on something slightly beyond the scope of the question. — Mike Seymour, Oct 23 '14 at 09:02

Tony Delroy · Accepted Answer · 2014-10-23T09:10:33.263

Is it a good practice to define nonmember function inside a class body with friend?

Indifferent practice, I'd say.

Any pros and cons?

Pros

the operators can refer to class members (nested classes, typedefs, enums, constants, static functions etc.) in the class's scope without needing to explicitly prefix them with the class name
it's convenient to have the streaming functions implicitly inline - no One Definition Rule hassles
friendship means you can access all the non-public members conveniently
people studying the class source code are more likely to notice the streaming capabilities
as Mike Seymour commented, if the class is a template then defining a friend lets you omit the template <...> aspect of the operators, and refer to the instance argument simply as a const Member& rather than a const Member<...>&.

Cons

you may want an out-of-line function definition so you can modify the implementation later and only need to relink (rather than recompile) client code
you're granting friendship that might not have been functionally necessary, that lessens encapsulation and therefore maintainability
people looking for a non-member streaming operator might not think to look in the class code
you could argue it "clutters up" the class definition source code, making it harder to take in the totality of the actual class members

As usual, the benefits of clean separation of interface and implementation - both for managing physical dependencies (the need to recompile rather than just relink) and for human readability - tends to increase for low level libraries used by disparate higher level libraries and applications, and be far lower for "private" support of local implementation (e.g. a class in an anonymous namespace in a .cpp file, used only by that single translation unit, or - even more so - a private nested class).

Yep, always keep in mind that classes are for encapsulation (reduction of dependencies). http://stackoverflow.com/questions/1692084/how-non-member-functions-improve-encapsulation — Deduplicator, Oct 23 '14 at 08:44

score 4 · Answer 2 · answered Oct 23 '14 at 08:39

In general, it's not good practice; ideally, the implementation wouldn't even be in the same file as the class definition. (Ideally, too, we wouldn't have to declare the private parts in the header file either.) There are a lot of justified exceptions, however:

The most obvious is in really simple helper classes, where there really isn't enough to justify separating both parts. This is especially true if the helper class is defined locally, in the source file, rather than in a header.
Another case is for friends, particularly in templates. If I write (even in a template) friend void f( MyClass& ), then I have declared a non-template to be friend, and I have to implement a separate non-template function for each instantiation type. If I provide the inline implementation in the class definition, however, the compiler will automatically create the separate non-template function for each type it is used. This is a very frequent motivation for definition operator>> and operator<< in the class, as they cannot be members; often they will be declared as friend even if they don't need access to private members, just so that they can be defined this way.

Finally, if there are no other declarations of the functions or operators, they are only visible within the class, or with ADL. Which shouldn't be a problem, as long as the function has at least one parameter which involves the class.

If you go for the ideal, wish for proper modules (which means no need for header-files at all). The committee is working on it... — Deduplicator, Oct 23 '14 at 08:40
@Deduplicator Certainly. But until then... (I argued strongly that we needed `export` for templates, given that we didn't have a concept of modules. I was also sort of against namespace, on the grounds that it only did a halfway job, and that we needed modules.) — James Kanze, Oct 23 '14 at 08:47

score 1 · Answer 3 · answered Oct 23 '14 at 08:28

1

Possible Pro: Easier to read and maintain, if you have all or most other functions handling the class' private members defined inside the class body as well. It keeps things together.

Con: Functions defined in the class body appear in every compilation unit instead of just one compiling the respective .cpp file you could put them in instead.

answered Oct 23 '14 at 08:28

Raoul Steffen

527
4
10

That's not really a con if the function should be inline because performance, or it is templated. – Deduplicator Oct 23 '14 at 08:38
Actually, in large projects, it's a lot more difficult to maintain. One of the first rules is that the interface definition needed by client code be maintained in a separate file from the implementation code, so that modifications of the implementation aren't visible to clients. – James Kanze Oct 23 '14 at 08:40
@JamesKanze: In the example, coupling is strong enough that separating out the definition does not buy anything at all, instead only making more work. Of course, there can be a difference when coupling is reduced. – Deduplicator Oct 23 '14 at 08:49
@Deduplicator: *"separating out the definition does not buy anything at all, instead only making more work"* that ignores the common recompile vs. "just" relink consequences of inline vs. out-of-line (and in implementation file) definition. "Physical" dependencies as Lakos calls it / "recompilation firewalls" and all that jazz.... – Tony Delroy Oct 23 '14 at 08:56
@Deduplicator I don't follow you. Implementation is implementation, and the less of it that is visible to the client, the better. (On large projects, you generally can't change the interface without architectural review, since this will break code all over the place. The simplest way of ensuring this is to put the interface and the implementation in two separate files, and to not check out the file with the interface unless you're authorized to change it.) – James Kanze Oct 23 '14 at 08:58
Ok, should have been more like "The coupling is strong enough and the implementation simple enough". – Deduplicator Oct 23 '14 at 09:01
@Deduplicator: About inline for performance or template reasons: You can always put you code further down into the header file, and still get the inline performance and template possibility. What I meant was, that you don't have that choice any more when you decide to put it into the class definition. Maybe I should have been clearer about that. – Raoul Steffen Oct 23 '14 at 13:03

Is it a good practise to put non member function inside a class?

3 Answers3

Pros

Cons