OOP: Advise vs. Prohibit -- Which is better?

This came up on the Drupal issue discussion area but it’s a larger issue - larger than PHP but of the languages discussed here at Sitepoint PHP is the one most likely to deal with this since it has scope regulation operators (Ruby may as well, but I’m not familiar with it).

They are currently hashing out which methods should be labeled @api or @internal, with the intent of warning the developers as to which methods can be interacted with, and which ones should be left alone.

Now it so happens I have written an assertion that checks the calling class of a method that can be put in to programmatically enforce that a class outside of a package file cannot call a “public” method - in effect making the method protected in the Java language sense of the keyword.

This was soundly scolded down, and I don’t have any real standing in the community so I just stepped away to let them do what they want to. But I don’t think it’s sound policy. The private and final keywords exist for a reason - to prevent code from being written in the first place that might cause a headache.

To my mind it is far better to be cautious, declare a class or method final, set a method to private, or use the protected assertion to package lock a method than to leave it wide open for extension and calling. In the former case if you find out that being able to extend the class or call the method is really necessary then you can simply lax up on the restriction in a patch and move forward from there. But in the latter case, if you find it makes sense to change the class or method in a significant way you won’t be able to without breaking any code that extends from or calls the class. In a large project like Drupal which is the foundation for other projects this sort of cross-checking is impossible.

I’m of the opinion that restrictions protect progress by making what you can and cannot change clear. The fewer interactions you need to maintain the easier it is to improve the code.

I find simply phpdoc labeling things to be as useless as PHP 4 era marking protected members with _ prefixes. Programmers, particularly junior ones, tend to ignore warnings and conventions like this and build code attached to them anyway. It’s far better just to say “no” and be done with it.

Otherwise why bother with scope limiting? Build that house of cards - make everything public!! Getters and Setters are for wimps, let things alter the class members at will right?? /sarcasm.

3 Likes

I agree with the prohibit stance. If a class shouldn’t be extended, then it shouldn’t be built to be extended.

Scott

It might be worth looking into how mocking frameworks behave when mentioning final
eg. in phpunit( quite an old version, but I think there is compiler limitations that stop this being any another way)

https://phpunit.de/manual/current/en/test-doubles.html

PHPUnit\Framework\MockObject\Generator
look for

 $class = new ReflectionClass($mockClassName['fullClassName']);

	if ($class->isFinal()) {
		throw new PHPUnit_Framework_Exception(
		  sprintf(
			'Class "%s" is declared "final" and cannot be mocked.',
			$mockClassName['fullClassName']
		  )
		);
}

Usually mocking frameworks dynamically extend of a class and then dynamically shadow the methods in the child to add the response listeners etc. with reflection. If the class or method is final then it cannot be overridden and mocked. The only way I know how to get around this is if class analysis is done in another process and then return a spec in json etc to then create the mock off, that then stops the autoloader ever being. I was thinking this would be a good candidate for using the built in web server.

You can do a dump of the full unevaled code in the

“protected static function getObject”

, if not there look for the eval.

It is quite an old problem. I don’t really do much php any more so there may be cool trustworthy ways to get around these problems.

So this may be one of those cases where perfect is the enemy of good.

The only classes requiring mocks are abstract ones, and don’t think PHP will allow you to make a class both abstract and final - the terms are mutually exclusive.

I’ve seen people do mocks in order to expose otherwise protected and private methods - but you shouldn’t be unit testing your internal methods. Your unit tests should deal with your code unit in the same manner that real world blocks of code are going to need to do, otherwise the test isn’t realistic.

EDIT: If you’re using object composition then typehint against interfaces, not classes. The mock then implements the interface for testing instead of mucking with the live class.

1 Like

It actually depends what school of testing is being proscribed to which actually determines how mocks are used. There are London and Chicago schools of TDD, I am London so in that case anything is potentially a mock for dependency injection. But that is a lifestyle choice of how I choose to work, and quite a lot of other people work as well. If the mocking technique is too ugly then a veering to Chicago maybe more evident.

The have an interface for every implementation is also a nice ideal but can lead to a single Impl for every interface which in turn makes things messy. This used to be an old restriction in some non php mock engines but the mess it causes makes mocking less popular. A bit like Javas checked exception creating more mess than they were supposed to solve and never being repeated. I was looking at some spring stuff yesterday that was just MonkeyImpl implements Monkey for about 500 lines in the xml.

The abstract class mocks tend to rely on the partial mocking mechanism but I haven’t personally used partial mocking in 4+ years as it gets very ugly. The complication of the test ends up greater than the code it is testing. That comes from teaching people it and finding it caused the greatest difficulty for everyone involved to debug. I always have the viewpoint that the “cognitively simple” tests the complicated and in many cases the code tested those tests.

Though anonymous classes which will be in 7 are much better for this kind of thing as faking of the the abstract methods can be done much clearer which is why Sebastian Bergman changed his vote on them, if I remember correctly that is. https://wiki.php.net/rfc/anonymous_classes

How Drupal internally mock is there own politics but there are many view points on this. DHH will say no mocks, just do high level integration tests but that might be the Ruby doesn’t mock that well with its much higher level of dynamism. Why I personally do Scala now as my first language as it has a lot of the flexibility but the compiler and really cool tripped out type system adds maintainability.

The creation of testable software is a bit of a thing for me like Tom and MVC. I have done TDD for 8 years( though a wise man would reply to anyone who uses year count “how many years of those were repeated” :wink: ) and have used Simpletest/PhpUnit/Phockito/Mockito/JUnit/Specs2/ScalaTest and been hit by many pitfalls along the way and a LOT of complaints whenever I try and make anyone do anything that relies on too much idealism to the point it sounds like “Dad do we have to write those tests, can’t we watch Spiderman first”.

On this statement
“I’ve seen people do mocks in order to expose otherwise protected and private methods - but you shouldn’t be unit testing your internal methods. Your unit tests should deal with your code unit in the same manner that real world blocks of code are going to need to do, otherwise the test isn’t realistic.”
I completely and utterly agree… Gives me shivers thinking about it.

1 Like

Totally agree with this. It’s a shame PHP didn’t use Java style ‘protected’ visibility and this wouldn’t be a problem, although historically this is impossible because the protected keyword existed long before namespaces did. Private classes would also help with this kind of thing.

1 Like

If it can go wrong, it might go wrong

It seems like common sense really, but I always try to remember that phrase. If you are ever in a situation where you can remove the capacity for something to go wrong/be misused, then do it!

So yeah, I completely agree with @Michael_Morris on this.

1 Like

I like that thinking @RT_.

In the lean production world (originating from Toyota), there is a Japanese word used to describe just that called Poka-yoke. It basically means, “Design the production system, so mistakes can’t happen.” :+1: If you consider putting classes together to make a whole application like putting parts of a car together to make a whole car, then the idea of poka-yoke is you won’t let the “operators”, or the client programmers, make (stupid) mistakes, when “putting the application together”. I think the analogy fits perfectly.

Scott

1 Like

Easy way to remember that:


:stuck_out_tongue:

1 Like

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.