[C++] friend keyword - bad programming style?

Yes. Why not just make fields public?

It doesn't come down to the friend keyword which is perfectly valid. It comes down to OO design.

Many languages do not support hacks like that. Once something is private, it's private.

Get/set routines don't solve anything either, they are just a different syntax for friend.

You might want to reconsider the design. While adding friend for certain specific small parts is just fine, abusing it is bad. Reason for this is that it doesn't lend itself to extensibility. And since your design isn't extensible, might as well put everything at same level.

If you have lots of classes with lots of friend accesses, you might want to put these classes to internal use only, and make everything about them public.

Quote:

is using the friend keyword considered bad OOP style

Technically, it's not even OO. Strict OO doesn't allow access to non-visible members at all. In reality however, it's practical to have that ability.

friendship is directed destruction of encapsulation (it provides a third party access to implementation detail), and as such should be used with caution. In complete isolation, on its own merits, friendship isn't neccessarily evil or wrong -- what makes it evil or wrong is the context.

However, if you're using it often, that probably means you're using it poorly.

Quote:

I need access to these private members from within the main world-class, thus I used the friend keyword. But I am not too sure whether this is good coding practice or not.

This is good example of where friendship is a bad idea. You're exposing implementation detail to objects that really don't require it; all you're gaining is a bit of short-term convienience. Provide a more robust public interface for the rigid bodies instead.

simple answer: NO!

same goes for GOTO:) to all those who hate goto's: basically even the most advanced OOP code is a collection of goto's :D

now - if u find yourself really abusing of such features, i mean if u know it's a better way to do things then it's bad practice.
by 'better' i mean something not born out of laziness and "i'll change it later, this is just for testing" type-of thinking

'friend' isn't bad itself, but it's often a symptom of bad design elsewhere. If you've got a load of private data and some code needs to operate on that data, that code should be a member of the same class (usually).

Friends are good! (Well..yes and no)

Why?

Well there are 3 reasons to use the friend keyword:

1. Sometimes it's the only way to do something


2. Sometimes it's the cleanest way to do something


3. Most times it's simplest way to connect classes within an existing poorly designed code base.

Point 1 is quite rare you would be hard pressed to think up times when there is little to no alternative. You might (or might not) consider operater overloading to be a case for this.

Point 2 is occasional, this is the inteded use of the friend keyword, rather than break encapsulation you actually increase it; the alternative would have been public get/set methods.

Point 3 is the most common use of the friend keyword, if you need two classes to communicate within already poorly designed code then this is the easiest way to do it; there's undoubtedly a better way to achieve the same thing but it requires redesigning your code, not an option if you're on a tight deadline. You may be able to avoid using friend in this case but only at the expense of writing yet more bad code.

So why does everyone say they're bad? Read on.

Many people who use friends excessively have a smelly design (as in point 3) and don't realise this; and so they assume that in their case they are correctly using friends as per point 2, of course they one day come back to their code and think 'oh-my, what an awful design' [wink]

As a rule of thumb if you're using the friend keyword and you don't know if it's the best thing or not then probabalistically you fit into point 3, this is why we generally urge to avoid them and instead consider a better design.

In your case it's likely that you're missusing the keyword, a better public interface is likely the best option.

In general if you have a bad design there are things you can consider which may help improve it:

• If accessing a private function then consider whether you should make it public.
  
• Consider whether the data that one friend class is accessing from the other would be better off in the accessing class in the first place.


• Consider whether the function that one friend class is performing using the data from the other class would be better off as a public method of the other class.


• Consider whether you definately require two classes, perhaps you're needlessly splitting a single responsibility between two classes.


• Look at your problem from a higher or more abstract perspective, ensure it is well defined. Perhaps there's an existing design-pattern you can employ.


• Research how others have solved your problem.

[Edited by - dmatter on September 17, 2007 11:26:05 AM]

IMHO, the whole idea of 'friends' just breaks the purpose of OO. Might as well go back to C instead of C++.

Whenever I am tempted to use them, I sit back and look at the design, and so far there has always been a design solution. I don't use them.

--random

I like the way c++ faq lite talks about "friend"

Quote:

Original post by janta I like the way c++ faq lite talks about "friend"

Thanks for the excellent link, also illustrating that the correct use of 'friends' within an OO paradigm is something best left to experts. I know that the C++ Standard Library uses them, and Stroustrup gives copious examples, but IMHO their use is best left to more advanced users, who understand the 'nuances' in an OO environment. In this thread, just about all the posters are in favour of their (infrequent) use, but with a lot of caveats.

I do concede one point of the article, though, that is that if the only alternative is to have public data in a class, the better alternative would be to use friends. In this way they do enhance encapsulation.

--random

Quote:

Original post by jpetrie This is good example of where friendship is a bad idea. You're exposing implementation detail to objects that really don't require it; all you're gaining is a bit of short-term convienience. Provide a more robust public interface for the rigid bodies instead.

Well, the problem is, that the world-class is responsible for collision detection and response, it detects collisions using the vertex data of the rigid bodies (this is why I need the friend keyword). I can't make the vertex data public, because it has to be constructed in a special way using AddVertex methods (to determine concave polygons and split them up into convex ones), so making them public could mess with the simulation.

Everything else has already public accessing methods.

I guess I then have to move the collision code inside the body class, although I didn't want the collision detection code to be public =/

Thanks for all those replies, I will now rethink my design to eliminate those "friends".

I didn't have to use the friend keyword before, as I am usually working alot with interfaces and the implementations have the actual data, so I can make more things public without thinking that improper use could break something (I don't mean making the data public, but several methods that otherwise would be private).
If the body-class were to be a interface I would give the implementation class the collision detection code and in actual use it would seem like the world class was handling the collision detection. But because of performance reasons I decided not to use inheritance with virtual classes. Accomplishing the same without interfaces seemed like making the collision detection private and defining the world class as a friend to the body class, because I didn't want the collision detection to be accesible from the outside as the world class should be responsible for handling collisions.

Edit:

Quote:

I do concede one point of the article, though, that is that if the only alternative is to have public data in a class, the better alternative would be to use friends. In this way they do enhance encapsulation.

Hey, that was my reasoning why I ended up using the friend keyword. Now I don't know if I should keep the keyword or not =/

I don't know if you are familiar with C++ patterns, but maybe you should look at the observer pattern. This might be appropriate to your situation, and could make the code more maintainable and more extensible.

Also you could use composition rather than inheritance, to reduce inheritance dependencies.

--random

Real OOP concerns itself with things like SRP, OCP, DIP, ISP and LSP; friend is an implementation detail. Friend can enhance things like SRP by allowing non-core functionality of a class to be implemented by non-members functions; such as formatted insertion operators. On the other hand friend can work against things like OCP, by carelessly opening the class to modification. As such a blanket denunciation or endorsement isn't possible.

Concrete example of using friend to improve code quality: let's say you have an interface that has functions that need to be called between two other functions. You can see this pattern played out again and again in things like DirectX interfaces, certain kinds of file locked operations, etc. One way to present such an interface looks like:

class Interface {  public:    virtual void Begin() = 0;    virtual void End() = 0;    virtual ErrorCode FunctionA() = 0; // must be called after Begin() and before End()    virtual ErrorCode FunctionB() = 0; // must be called after Begin() and before End()};

This depends on the caller to manually make sure that Begin() and End() bracket all the calls. Now consider an alternate formulation of the interface:

class Interface {  private:    friend class Lock;    virtual void Begin() = 0;    virtual void End() = 0;    virtual ErrorCode FunctionA() = 0; // must be called after Begin() and before End()    virtual ErrorCode FunctionB() = 0; // must be called after Begin() and before End()};class Lock {  public:    Lock(Interface & interface) : interface_(interface) {      interface_.Begin();    }    ~Lock() {      interface_.End();    }    ErrorCode FunctionA() {      return interface_.FunctionA();    }    ErrorCode FunctionB() {      return interface_.FunctionB();    }  private:    Interface & interface_;};

Here the access to Interface is protected by a gateway Lock. In order to call any of the member functions on Interface, you need a Lock object, which in turn, enforces the call of Begin() and End(). If you don't have a lock object then you can't call the functions.

(N.b.: example omits implementation details like copy constructor and assignment operators)

Quote:

Original post by Cranky Hey, that was my reasoning why I ended up using the friend keyword. Now I don't know if I should keep the keyword or not =/

You have to determine whether you really need those classes to share data, or whether you are instead mixing up responsibilities too much...again, this is more of a design function/pattern question rather than a specific code issue.

--random

If you find yourself needing friend classes to access private member data, consider simple set and get functions that are public. This way you can control the data going in and out as necessary.

The only place I really can't resist using a friend class is in my graphics wrapper classes. Say I have a Texture class than contains an IDirect3DTexture9 pointer and a GraphicsDevice class that wraps the IDirect3DDevice9 pointer.

Obviously the GraphicsDevice needs to be able to access the IDirect3DTexture9 pointer internally inside, say, its SetTexture() method. But I don't want to expose the pointer through a method of the Texture class, or provide any other parts of the public interface to Texture purely to make it interact with the GraphicsDevice, since that also exposes it to everything else.

So I have



Others might be able to suggest other solutions to this, but it works for me. Texture doesn't have to either expose its internal pointer or any otherwise unnecessary methods that only the GraphicsDevice would ever need to call.

I personally think that in the case of such tightly coupled and related classes as above, use of friend provides very clean solutions.

Quote:

Original post by Cranky Well, the problem is, that the world-class is responsible for collision detection and response

That responsibility probably shouldn't reside there, unless that's all the world-class does in which case the name is a bit misleading.

Quote:

it detects collisions using the vertex data of the rigid bodies (this is why I need the friend keyword). I can't make the vertex data public, because it has to be constructed in a special way using AddVertex methods (to determine concave polygons and split them up into convex ones), so making them public could mess with the simulation.

Well the rigid body can generate the vertices however it wishes and then provide a getCollisionVertices() public method that will return them. You don't need to make the vertices themselves public.

Quote:

Everything else has already public accessing methods.

Are you saying all the data members have associated accessing (get/set) methods?

Quote:

I guess I then have to move the collision code inside the body class, although I didn't want the collision detection code to be public =/

What about a class dedicated to performing collision detection on rigid bodies?
One body may be able perform collision detection with another body provided the class has access to enough information, it may not know its absolute position for example.

Quote:

If the body-class were to be a interface I would give the implementation class the collision detection code and in actual use it would seem like the world class was handling the collision detection. But because of performance reasons I decided not to use inheritance with virtual classes. Accomplishing the same without interfaces seemed like making the collision detection private and defining the world class as a friend to the body class, because I didn't want the collision detection to be accesible from the outside as the world class should be responsible for handling collisions.

Are you talking about pure virtual base classes as interfaces (or a Java style interface)?
When I say 'interface' I simply mean the methods that belong to a class, typically the public ones. Pure virtual classes simply enforce an interface and allow you to use it through polymorphism; inheritance and virtual methods aren't required for a class to exhibit an interface.

Quote:

Hey, that was my reasoning why I ended up using the friend keyword. Now I don't know if I should keep the keyword or not =/

There's likely a better design with a more sensible use of friends; 'a more sensible use' probably means 'not using' [smile]