public or protected?

Discussion of testing theory and practice, including methodologies (such as TDD, BDD, DDD, Agile, XP) and software - anything to do with testing goes here. (Formerly "The Testing Side of Development")

Moderator: General Moderators

Post Reply
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

public or protected?

Post by Ambush Commander »

I am currently in the process of converting a PHP 4 codebase to PHP 5, this means fully specifying PPP (public/protected/private).

Most of the time, the code helpfully points out in the docblock what visibility I should assign to a method. However, I'm frequently running into cases where the code claims a method is protected, but it's invoked by the unit test in a TDD-esque style into order to be tested.

Ideally speaking, I should probably factor these out to a separate class and treat them as public. However, as of right now, I'm simply trying to get all the PPP's set so I can commit to the repository (and, also, so that the code doesn't throw noodles of E_STRICT errors when I run PHP 5.0). Should I simply give in and keep them as protected, or should I protect them and either 1. throw out their unit tests or 2. create a subclass that has a public interface for the protected method?

Thanks.
jmut
Forum Regular
Posts: 945
Joined: Tue Jul 05, 2005 3:54 am
Location: Sofia, Bulgaria
Contact:

Post by jmut »

Well it all depends on time you have....I personally will make it php 5 strict first...meaning all public..which will keep php 4 scope and won't break code...once this is done...and all other php5 peculiarities...I will refactor so classes are more cohesive.
alex.barylski
DevNet Evangelist
Posts: 6267
Joined: Tue Dec 21, 2004 5:00 pm
Location: Winnipeg

Post by alex.barylski »

I'm not sure I'm following you 110% BUT...

I thought the idea was to test public API only, in which case, I would suggest making the methods protected and dropping the unit tests as they are dedudant in a way are they not?
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Post by Maugrim_The_Reaper »

It can depend on whether other code relied on PHP4 methods always being visible, or if method visibility was not considered as such during some part of development by someone. But yes, E_STRICT first, then do some refactoring to figure out which intended protected/private methods are being assumed as public by other parts of the system and make the necessary changes. I would only delete such unit tests gradually and check you have code coverage from the remaining tests.
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

Sounds like you might have a bit of work on your hands there. I don't think anyone here is going to be able to suggest a way of getting round this problem that isn't going to involve looking at each problem on an individual basis and making some pretty big changes where necessary.

Bare in mind: if a method doesn't have side effects or affect it's object's state (or can be made that way easily) it can, with all likelihood, remain public even if it's not intended to be part of the public API without too much harm.
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

(Hmm... it seems like topic reply notifications aren't working)

I've upgraded all the identifiers, using protected when it didn't break unit tests and public when it did (adding a @todo Make protected when I thought it wasn't very appropriate).

I've always thought making methods private/protected was a way of damage control: since no one else is allowed to use them, you can refactor with impunity (well, in the case of protected variables, you only have to worry about subclasses). It's not necessarily always practical to achieve 100% code coverage without testing some of the lower level functions, which makes me think they should be refactored to their own class. I'm also, however, trying to make sure my class count doesn't explode anymore than it is already (classes take up a lot of memory!)

Question: Is it a good practice to redefine protected methods as public in a "Testable" sub-class to unit test them?
jmut
Forum Regular
Posts: 945
Joined: Tue Jul 05, 2005 3:54 am
Location: Sofia, Bulgaria
Contact:

Post by jmut »

It is hard to believe for me those unit tests are good enough to rely on if should be public or protected..but who knows. It depend on scale of application.
Generaly I believe only interface should be tested...no private/protected should be part of tests.... only everything that is visible (public properties/methods)
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

Question: Is it a good practice to redefine protected methods as public in a "Testable" sub-class to unit test them?
I don't see any reason why you couldn't do that. In fact that's probably a very good idea.
jmut wrote:Generaly I believe only interface should be tested...
Hmm yeah, generally.
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Post by Maugrim_The_Reaper »

Question: Is it a good practice to redefine protected methods as public in a "Testable" sub-class to unit test them?
No it's not. Think of it this way - a class is a set of valued behaviours accessed from the public interface. Now, the important thing is that the behaviour is valuable, the implementation code is not (in fact it's generally extremely dynamic and changes as often as the Sun rises). Testing for the behaviour creates a valuable test (we certainly want to know if something valued breaks). Testing the implementation is literally duplication and pointless - if the behaviour is tested, and it relies on the implementation, then testing the implementation means we're doubling up (it's already executed by the tests of public behaviour if you check their code coverage ;)). Also since implementation can (and should, i.e. refactoring) change often it means such tests will always break. This leads to another problem - tests which break often are a waste of time without a really really good reason. If tests aren't write once and forget then we're going to spend a lot of them keeping them fixed.

I'd suspect testing protected/private methods is likely the number one problem with test suites in PHP today and you can spot it a mile off by the incredible number of tests even a small project ends up producing. Also subclassing just changes the SUT anyway - you're not really testing the original class anymore by cheating ;). Lastly - let's face that by cheating you're skipping all the code smells testing gives away. If it's not testable off the bat then it's a pretty strong signal you need to fix the interface - most of the time that leads to simple dependency injection or flexibility problems that should be resolved.
I've always thought making methods private/protected was a way of damage control: since no one else is allowed to use them, you can refactor with impunity (well, in the case of protected variables, you only have to worry about subclasses).
The way I see them is that they're not important to the end user. All the user cares about is how to turn Lead into Gold (the internal process of the conversion is not worth examining unless it can be reused to turn Water into Wine ;)). You can care about the internal process if it's delicate enough to warrant attention - usually the point when you see Mock Objects appearing to verify the process itself - not just the public behaviour.
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Post by John Cartwright »

Wouldn't testing the protected/private methods of the class make it too fragile, possibly beyond a point where it makes refactoring counterintuitive??
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

Try and "forget" what the class currently has, and create a test for the expected behaviour of the object. Remember, you are only testing what happens on the "outside" or rather to external objects, not what happens on the inside. :)
Post Reply