Java Methods: protected is the new private?
In code reviews (including my code), when I see a Java method marked as private, I ask if there are any unit tests for it.
Since tests are written in other files in parallel packages, the answer, of course, is no.
I've recently realized that I've obtained the following habit: my methods are either public or protected. I have no use for default or private. I don't like default because I feel compelled to write a comment saying that it is package-level access; I don't like private because of the lack of testing options.
Before you start hammering in comments, here are some thoughts:
- This is not for a formal API to another team or other 3rd party. In that case, I would be more careful and rigid. You may argue there is no distinction, in which case: commence hammering.
- I realize that
protectedis notdefaultand that it is leaking encapsulation somewhat. I can cheerily say that I don't care. I like scanning a file and seeing eitherpublicorprotected. - I'm not trumpeting that I write more unit-tests than others on my team. Just because a method is
protecteddoesn't mean that I've written the tests!
13 comments:
I occasionally find myself making internal methods more accessible, but it's not my norm.
My reasons:
* I mostly write smaller classes, so the internal methods are small, and rare.
* For me a "unit" is not a method, so I'm trying to test a class (or sometimes a set of classes) which doesn't mean testing every method specifically and individually.
* I'm typically testing that a class fulfils its contract, and that is defined by the public (or occasionally protected) methods. The internal (private) methods are an implementation detail that don't need to be explicitly tested. I've most likely pulled that code into its own method because either:
(i) It improves readability.
(ii) It's used in more than one place.
Neither of those reasons force me to write explicit tests for the method.
When I find myself wanting to make an otherwise private methods protected (or public, or default), I treat it as a design smell. Often that means that there's another class (and interface?) in there trying to get out. If the functionality is definable enough to make a method for it, and important enough to want to write a test for it, then it's probably important enough to be a class of its own.
As I said, I do it sometimes (including yesterday), but not as a matter of course.
One of the points of unit testing, for me, is to have a set of tests that enforce, as Tim says, the contract that the class provides. This allows me to be able to refactor the class as much as I want, without worrying that I'm breaking something (since the tests still pass). That refactoring will almost certainly involve pulling some code into private methods; that's a good thing. The point is that my tests are hopefully good enough that the functioning of those private methods are being tested by testing the class.
I also think that someday, someone else is going to interact with your code, even if it isn't intended as a third party API. It might even be you, two years from now (having forgotten what you had in mind). How will this person know which methods are supposed to be called from other classes? Making a method public or protected is, aside from the security and encapsulation effects, a way of making a statement about your intentions for how that method will be used.
I am not saying that I do this, but one good consequence of TDD is that you won't end up with these private methods. When you refactor the original location of the method that got too big, you would have to move it to it's protected location and put the test in place.
private methods are functional decomposition by definition, which is an antipattern for the many reasons. But it is hard to know what the class should be for just helper methods.
I make things as private as possible. After consulting at the same site for many years and actually supporting a lot of my own legacy software, the lesson is -- if a method is visible, programmers WILL call it. And once other apps start using your code, it's damn hard, if not impossible, to take things away.
+1 for package-private. Embrace the brevity! Use /*package*/ if you're that worried about being explicit, but using protected is too distasteful.
Not sure if you have looked at Mocking frameworks that help you work around language restrictions for testing purposes like JMockit, Powermock (there are others too).
Their homepages explain why private, static methods/fields are useful for design purposes and I agree with that. Ofcourse, with great power comes ...
Thanks for the responses, everyone...
Certainly, the answer to the post is a resounding no.
re: use smaller classes. I should have mentioned that I deal regularly with large, legacy classes. IMO, it is easy to criticize the premise of the question ("well don't have private methods") but that is unavoidable in practice.
re: mocking. Having recently been seduced by Mockito, I thought that this might be a great point but my understanding is that we should not mock a class under test (versus a collaborator). I think mocking definitely allows interesting 'bending' of access modifiers for collaborators; I'm not convinced for the class under test.
re: brevity. I wanted to mention this point: Groovy and other languages celebrate the 'low ceremony' of brevity and convention. It is interesting to me that people rarely mention the default modifier in that context.
The upshot is that I should re-evaluate my habit with this feedback. To be candid, it may well become a mumpsimus in certain contexts. (I'm not proud of that, but there you have it.)
This has made me think. Too many user stories and thoughts of TDD push you down the road of top down decomposition which leads you to refactor code into private methods.
What happened to Object Orientation? Design Patterns are resorted to only when "justified". So I am going to try to think differently about problem solving, instead of drilling down, I am going to let the solution bubble up and compose the objects instead of decompose the functionality.
Sorry, I'm not getting it.
If a method is private, doesn't that mean that it's essentially inlined in at least one package/protected/public method that is already being tested?
As mentioned by another commenter, private methods do have a smell, but I think if you're using one, and everything else is being tested properly, you are also implicitly testing your private method.
Private methods, for me at least, are just a way to avoid copy-paste coding (if the method is called more than once ... ) and/or make the code slightly more legible. I don't use them often, but when I do I know they're already being tested.
I will look for a deeper analysis that let me factor out private methods which contain logic in other classes. However, the reasons are a bit long to be written here. I have just put them in an article on my blog:
http://ossigeno.sourceforge.net/blog/content/article/respect-my-privacy-please
Go back to C++, clearly you miss friend classes.
And you are obsessed with testing mechanisms but not enough to know private methods can be tested if you know how to do it.
Michael, I love you but I'm not on the same page with you on this one.
Speaking as a Scala-ist, I find even private to be too open at times. Java private grants access to other instances of the same class. This is great if you're defining a "plus" method on a Complex number class for example.
But if I'm a private method on an object, I seldom want other objects to call me, even if they are the same type. I exist to serve my own unique instance. (Sorry for the anthropomorphizing.)
When I code in Scala, I've noticed that I start out making things private[this], which is even more restrictive than Java's private. As I discover I need to, I will loosen the access qualifiers. The "correct" amount of privacy/protectedness seems to emerge naturally over time.
But if I start out with a loose access qualifier, it's very difficult to tighten it back up later on, even if I really want to.
By the way, I'm intrigued that some find private methods a code smell. I'm also going to think and rethink about this, because to me they smell like roses.
Well named private methods support SLAP (Single Layer of Abstraction Principle), which greatly improves the readability of the code. For me, SLAP is a far more compelling reason to have private methods even than avoiding code duplication.
Folks,
Thanks for the comments... Just FYI, I moved locally this weekend and have been offline. I hope to comment again soon. I've definitely been mulling over the feedback... good stuff.
M.
Post a Comment