Conversion of all SFML classes from using private to protected

[Joshua Flynn]

In regularly using SFML, I often encounter a class where I want to expand it's basic functionality (a term for this is extensibility), but I'm unable to do so as it's contents and functions are declared private, which means only members of that class and friend classes can access it.

A prime example would be adding pause functionality to the sf::Clock.

Often, the individual is faced with a choice: either write their own class from scratch to address what is usually only a few additional functional add-ons, or port the SFML source code, make modifications at the low-level and then compile a new library, which then often makes any code reliant on it incompatible with the active version of SFML (and thus requires all future additions of SFML be modified in the same way, a time consuming process!).


A very simple solution, which comes at no cost to SFML and greatly increases it's functionality, is for SFML classes to change their private contents and functions, to protected, which allows a subclass to inherit and access both the functions and the contents, which would allow people to readily extend SFML whilst solving problems such as allowing people to add pause functionality.

It might be argued that private is there as a form of protection, however, given the user is forced into  implementing their own (dodgy!) design (either their own standalone class or modifying the library), it's safer if they're allowed to simply expand a pre-existing stable class.


For example, right now (ignoring the Pause example for sf::Clock) I'd love to implement my own append and get split channel style function for sf::Buffer, but it's looking like I will have to copy out sf::Buffer's contents, modify it, and then put it back in, which is pretty messy and would look neater as an expanded sub-class of sf::Buffer, but without direct access to it's internalised vector, will require I declare duplicate space (a big overhead on large sound files) to do things like append operations - noting that std::vector directly supports appending (where I can simply 'attach on' a sound file without duplicating sf:Buffer's contents).

I know the idea of shifting it from private to protected will get stick from those in-favour of the status quo, but my classes in my library use protected de facto to allow extensibility *unless* I can justify the usage of private (IE a function call must only be called in specific states that only the class itself knows).

[Hiura]

A very simple solution, which comes at no cost to SFML and greatly increases it's functionality, is for SFML classes to change their private contents and functions, to protected

Actually, there is one major hidden cost with this approach: everything that is accessible from outside SFML implementation code is de facto part of the *public* application interface. Hence, `protected` is somewhat equivalent to `public` meaning that we cannot change them without potentially breaking users' code. From a maintenance point of view, the modification you're proposing has huge drawbacks.

Now, I'm not saying that no function should be protected instead of private but this should be discussed on a case-basis.

[therocode]

Making everything protected by default IMO breaks a lot of design considerations. It is nicer when classes are either designed to be inherited or designed to not be. This creates an API which is more focused and clear in its purpose. Besides, to make them properly inheritable you also need to add a virtual destructor which would AFAIK cause a vtable to be inserted in every class which could be  a drawback in terms of overhead in some cases.

It is better to by default try to use the avialable components to build new ones with composition rather than inheritance. If you truly have issues using this way to build upon SFML's functionality, then maybe those classes are flawed in their design in other ways in which case it could probably be improved in better ways than making everything protected.

[Nexus]

In regularly using SFML, I often encounter a class where I want to expand it's basic functionality (a term for this is extensibility), but I'm unable to do so as it's contents and functions are declared private, which means only members of that class and friend classes can access it.

Yes, and this exactly the purpose of encapsulation. protected is almost public, because it exposes implementation details, and by doing so, tremenously limits the implementation.

This is the typical Java way of thinking, where classes are the core language element and deeply nested class hierarchies are the standard. Not so in C++ -- here, you have many possibilities of extending functionality, and inheritance is usually not the best option.

A prime example would be adding pause functionality to the sf::Clock.

It's indeed a great example, because it can be perfectly extended without inheritance, see thor::StopWatch.

You make a common mistake: you violate the Liskov Substitution Principle by assuming that stopwatches are clocks and have every property of their base class. Currently, a class invariant of sf::Clock is to be always running and to count the time since the last reset() call. Depending on the semantics you provide, reset() would need to be virtual, or you have a broken base class. The destructor would also have to be virtual, for people who destroy stopwatches polymorphically. This would add useless overhead that most people never use, but all people have to bear.

And this is typical: C++ classes must be explicitly designed as base classes, inheritance can't be an afterthought.


I can widely agree with Hiura and therocode. If you find a certain component in SFML lacking, you should report this to us, and not just patch your way around it. This way, other users can benefit from improvements as well.

[Joshua Flynn]

A very simple solution, which comes at no cost to SFML and greatly increases it's functionality, is for SFML classes to change their private contents and functions, to protected

Actually, there is one major hidden cost with this approach: everything that is accessible from outside SFML implementation code is de facto part of the *public* application interface. Hence, `protected` is somewhat equivalent to `public` meaning that we cannot change them without potentially breaking users' code. From a maintenance point of view, the modification you're proposing has huge drawbacks.

Now, I'm not saying that no function should be protected instead of private but this should be discussed on a case-basis.

Certainly. But I believe that if you're willing to expand a pre-existing class, it implies you have some understanding of how it works, a more 'advanced: proceed at your own discretion' kind of thing. A lot of users request features or functions that are limited case use, but can't be added without a big rewrite of either library or their own class. Having protected allows them to extend their own limited use case to that class without having to bug Laurent to add it in.

A prime example is the HTTP class: it doesn't do POST requests. When I first approached this I was having issues until I realised it always transmitted GET - which required I modify the class itself fundamentally. After much stress re-writing the SFML library and recompiling, I found that if I simply wrote my own HTTP class from scratch, I'd not have compatibility issues. But then it undermines the point of having SFML with HTTP (and my HTTP class isn't without bugs).

Another example is an extensive number of individual draw requests (something I haven't encountered yet), which, if it was possible to extend the Window class, it'd be possible to attach on a sort of push style bulk drawing system, so it'd go:

Push(Sprite);
Push(Image);
Push(Sprite);
//...etc
DrawPush(); //Implicitly clears push list
DrawPush(false); //Explicitly doesn't clear push list

I know no-one would necessarily agree with that, but if extensibility was there, no-one would have to. I think the idea of shifting to protected would help everyone: SFML can say 'you can extend that', and I can build on top of some good classes.

[Joshua Flynn]

Yes, and this exactly the purpose of encapsulation. protected is almost public, because it exposes implementation details, and by doing so, tremenously limits the implementation.

How does allowing expansion of it's implement 'limit' the implemention?

War is peace and all that.

This is the typical Java way of thinking, where classes are the core language element and deeply nested class hierarchies are the standard. Not so in C++ -- here, you have many possibilities of extending functionality, and inheritance is usually not the best option.

Inheritence *is* the best option, that's why it exists! That's why protected exists! This isn't a simple hate on Java style idea - given that platform is about 'pointerless' systems, but the fact C++ is an expansion on C with the idea of classes with intended inheritence. Indeed, the very class SoundStream uses this very assumption to allow people to write their own variants on it!

It's indeed a great example, because it can be perfectly extended without inheritance, see thor::StopWatch.

By rewriting an entirely arbitrary class that makes use of system clock calls... which makes sf::Clock now redundant to me. sf::Clock has all the components to become a pauseable clock, but it requires I have access to it's stored time variable from a sub-class perspective.

The fact it's been implemented by so many people suggests it should have been added ages ago, but rather than fighting SFML's paradigm, it's easier if I just say 'hey, can I just independently modify the clock?'.

You make a common mistake: you violate the Liskov Substitution Principle by assuming that stopwatches are clocks and have every property of their base class. Currently, a class invariant of sf::Clock is to be always running and to count the time since the last reset() call. Depending on the semantics you provide, reset() would need to be virtual, or you have a broken base class. The destructor would also have to be virtual, for people who destroy stopwatches polymorphically. This would add useless overhead that most people never use, but all people have to bear.

The sf::Clock is an example of why allowing access via protected would be a good idea (indeed, because it would require a subclass, IE Stopwatch would inherit from Clock). Because most people require a pause/unpause function, it follows the principle of least surprise that it should be added. But alas, my argument is not adding pause to clock: it's moving clock's variables etc to protected so I can derive a subclass with pause from it.

And this is typical: C++ classes must be explicitly designed as base classes, inheritance can't be an afterthought.

This isn't a refutement to the position given though. If anything, as a library, extensibility should be a prime consideration. Consider string basic under STD which is used for the groundwork of char and wchar classes, or the map/list/deque (which are all template types but extensible in their form).

I can widely agree with Hiura and therocode. If you find a certain component in SFML lacking, you should report this to us, and not just patch your way around it. This way, other users can benefit from improvements as well.

What might be lacking from SFML for me, might not warrant a wide enough use case scenario to justify SFML including it, which simply forces me to either write an entire (duplicate) class from scratch, or rewire the library, and neither are practical considerations.

But I'll humour, here's my list (I suspect you'll agree they won't be ending up in SFML any time soon, hence my request for extensibility so I can add my own changes):

1) Pause/Unpause for clock (I'm not the only one).
2) Clock arithmetic (being able to add or subtract clock time from one clock to another with a return clock class as the end result: specific case stuff. You'll definitely say no to this).
3) Ability to specify request types on HTTP (at the very least, GET types).
4) Base64 decoding support on HTTP (standard format for encoding files, messages).
5) Bulk draw support for Window.
6) The ability to 'append' sounds in sf::Buffer.
7) The ability to 'insert' sounds in sf::Buffer (EG silence).
The ability to overwrite/insert sounds in sf::Buffer.
9) GetChannel/GetSplitChannel functionality (so it's possible to isolate left from right cleanly).
[All of the sf::Buffer case scenarios are prime for subclassing]


I'm sure there are others but those are off the top of my end. It's a real bugbear for me to not be able to subclass pre-existing classes in SFML because it means I essentially have to create an external duplicate that then occasionally interfaces with the arbitrary SFML class I can't subclass, which is messy, bad design and inefficient - but I'm forced to do that because inheritance is a thing not being utilised.

There is a reason why protected exists, it's not 'just' a public, and that's misleading at best. The whole point of protected is to grant subclasses access whilst denying conventional outside usage. The fact virtualisation of functions exists implies C++ is all about the inheritance, especially given it's OO code.

[Hapax]

It's indeed a great example, because it can be perfectly extended without inheritance, see thor::StopWatch.

By rewriting an entirely arbitrary class that makes use of system clock calls... which makes sf::Clock now redundant to me. sf::Clock has all the components to become a pauseable clock, but it requires I have access to it's stored time variable from a sub-class perspective.

I don't think you actually looked at Thor's Stopwatch. It internally uses an sf::Clock.

Stopwatch would inherit from Clock

You are suggesting that a stopwatch is a clock, which is incorrect.

[Joshua Flynn]

I don't think you actually looked at Thor's Stopwatch. It internally uses an sf::Clock.

That's not evident from the HPP supplied. Perhaps in the CPP it is, but that's not been provided.

You are suggesting that a stopwatch is a clock, which is incorrect.

Then how does it keep tabs on the transition on time if there is no clock intrinsic?

The Stopwatch is a red herring besides: this is about using protected to allow extensibility of pre-existing classes. The fact you cite the stopwatch in the other library as making use of an sf::Clock internal, not only suggests it indeed does rely on a clock to that effect (refuting it's 'not a clock' argument), but a stopwatch should really be a subclass of clock.

Whether your agree or not is just grounds to allow customisation of classes as people see fit. The fact this won't ever be approved is why SFML requires the ability to customise SFML classes to some degree to use limited use cases. At the moment, sf::Clock is entirely redundant in my library because if I'm duplicating a majority of the functions of sf::Clock to formulate a pause/unpause feature, I might as well add the rest.

[As it stands, it makes no calls to sf::Clock.]

[Hiura]

Having protected allows them to extend their own limited use case to that class without having to bug Laurent to add it in.

On the contrary, after this private -> protected modification we are always bothered with those additional functions, which would now be part of the API but were previously implementation details, when we have to fix a bug or add a new feature.

I don't think you actually looked at Thor's Stopwatch. It internally uses an sf::Clock.

That's not evident from the HPP supplied. Perhaps in the CPP it is, but that's not been provided.

Have you looked at line 83? 

You are suggesting that a stopwatch is a clock, which is incorrect.

Then how does it keep tabs on the transition on time if there is no clock intrinsic?

There's a big difference between the is-a and the has-a relationship. You should read a bit on that and especially about the LSP as Nexus said. 

[Joshua Flynn]

On the contrary, after this private -> protected modification we are always bothered with those additional functions, which would now be part of the API but were previously implementation details, when we have to fix a bug or add a new feature.

What additional functions would you be bothered by? I don't understand the problem you guys are referring to - you make it sound like switching to protected means that you are obliged to somehow add in external third party functions... which isn't my argument. If anything, protected would allow you to say to more posters 'you can do that yourself by subclassing XYZ' - a basis to refuse third party functions.

I'd personally just like to be able to modify SFML classes rather than work on re-writes. Perhaps an alternative idea - could SFML offer a 'private to protected' version that's separate from the main build, but would install essentially the same way?

I've never seen SFML using protected before, and I think you should at least try it. I think it'd solve a lot of the basic complaints.

Have you looked at line 83? 

I have now, but that wasn't what was originally linked to above. My argument isn't reliant on my knowledge of third party libraries.

There's a big difference between the is-a and the has-a relationship. You should read a bit on that and especially about the LSP as Nexus said. 

Which would all be fascinating if this thread was actually about sf::Clock and stopwatches (which is a subjective design choice, if anything), when it was only an example of where subclassing could apply.

I notice for example no-one has broached subclassing SoundBuffer. Is that because it's easier to attack the stopwatch example using appeals to subjective priniciples? Does the clock example apply in the case of append operations for SoundBuffer? (I'm guessing the answer is no).

[You *could* avoid subclassing... but that doesn't incur the same overheads as image or sound manipulation operations where having direct access to the data is preferable to stepping around it because of some priniciple some guy wrote. Myself? I'd consider a stopwatch a subclass of a clock because it intentionally inherits the clock's characteristics - in the same way a std::string inherits a basic string on functionality.]

[Joshua Flynn]

To put it bluntly: just because subclassing is avoidable, doesn't mean it should be avoided.

The fact a lot of SFML classes internally are subclasses of other SFML classes, implies even when it comes to development and expansion of SFML that subclassing is required. But why is it then restricted from use by other developers if it's use is a requirement intrinsically?

[Satus]

In my experience, the larger you application becomes, the harder it is to deal with classes relationships and inheritance hierarchy. I had a lot of times when I thought "Could've use composition here, would've make things a lot times simpler". Also overusing OOP and classes often results in violating SOLID principles.

[Jesper Juhl]

What additional functions would you be bothered by? I don't understand the problem you guys are referring to

The ones that were changed from private to protected.
As soon as the function becomes protected it becomes callable by user code which means SFML can no longer freely change the implementation since that might break user code that calls it and depends on details of the previous implementation.

[Hapax]

Inheriting your "pausable clock" (i.e. stopwatch") from an sf::Clock means that you are saying that your stopwatch is a "clock that only keeps track of time passed with an ability to restart", since that is what an sf::Clock is.
Nexus mentioned this already:

You make a common mistake: you violate the Liskov Substitution Principle by assuming that stopwatches are clocks and have every property of their base class. Currently, a class invariant of sf::Clock is to be always running and to count the time since the last reset() call.

I don't see why it's so important to inherit from sf::Clock rather than to just use sf::Clock.

[Joshua Flynn]

Inheriting your "pausable clock" (i.e. stopwatch") from an sf::Clock means that you are saying that your stopwatch is a "clock that only keeps track of time passed with an ability to restart", since that is what an sf::Clock is.

Except my 'stopwatch' isn't called 'stopwatch' (which is where your assumptions are going horribly wrong), but 'AdvancedClock' with stopwatch-esque capabilities. Chances are I will expand upon that - with the clock arithmetic noted above. Is clock arithmetic a stopwatch only functionality? I'm guessing the answer is 'no'.

Nexus mentioned this already:

You make a common mistake: you violate the Liskov Substitution Principle by assuming that stopwatches are clocks and have every property of their base class. Currently, a class invariant of sf::Clock is to be always running and to count the time since the last reset() call.

LSP falls foul of the Circle-Ellipse problem. But what you've got to consider is that each person implements design of a particular feature differently. This isn't an argument of having a stopwatch in SFML... it's the ability to inherit from clock - regardless of what purpose it might be for. Stopwatch is just a 'tip of the tongue' example.

I don't see why it's so important to inherit from sf::Clock rather than to just use sf::Clock.

This isn't just about sf::Clock. It is but one example.

For me, if a class calls a duplicate function of another class (the stopwatch example you've given calls 'getElapsedTime' - which is a duplicate function of sf::Clock), I feel that's a signpost saying 'this class needs to be a subclass', especially if it shares variables and merely 'adds additional functions'.

If I pass my AdvancedClock (to you, a type of stopwatch) to a function, the function can still treat it as though a clock - because it still has those types of variables and functions in-place: getElapsedTime, restart etc.

If I put a clock internal to my AdvancedClock, I cannot use the same functions that could call a clock - despite there being plenty of overlap in accessibility and function usage!

It's quite possible to argue either way. I'm not asking you to accept nor use inheritance in your clock classes, nor anyone else: I'm asking for the ability for myself and others to be able to use inheritance in general.

[It'd allow people to readily expand upon SFML as well - even offering third party libraries that explicitly expand SFML classes. And even that's not your thing, you'd be making my life a lot easier, and probably many others as well.]

The ones that were changed from private to protected.
As soon as the function becomes protected it becomes callable by user code which means SFML can no longer freely change the implementation since that might break user code that calls it and depends on details of the previous implementation.

Ah, there, a valid response to the actual topic.

This is an invalid rebuttal, however, because SFML has often changed things that go on to break user's code - for example, shifting the naming convention from UpperCase style to lowerUpperCase style, the removal of rand etc. I think generally, as a rule, when one uses a codebase under development, they have to expect code breaking changes.

Changing how getSampleRate() works is no different if it was a private, public or protected function. Indeed, any changes to the original function would apply less in the case of a subclass - which often overloads the original function.

I think not wanting to change something because it might break code isn't legitimate here, because one could use that to argue for a stagnant SFML with no changes. I wouldn't mind if they changed something fundamentally - because with protected access, chances are I could easily add in a work around.

I've now got three classes out of SFML with dopplegangers due to the inability to inherit: Clock, HTTP and SoundBuffer.