Keep Your Methods Private and your APIs Minimal
I was reminded once more today just how important it is to write minimal APIs that don’t expose more than they have to. Briefly I had code like this:
private boolean flag;
public boolean getFlag() {
return this.flag;
}
public boolean setFlag(boolean value);
this.flag = value;
}
Pretty boilerplate stuff, I think you’ll agree.
However I noticed that after some refactoring that merged a couple of classes I was now only calling getFoo() from within the same class (or at least I thought I was) so I marked it private. Eclipse promptly warned me that the method was unused so I deleted it. Then Eclipse warned me the field was unread. That seemed wrong so I looked closer and yep, it was a bug. The feature the flag was supposed to control was always on. During the refactoring I had failed to move the use of the flag field into the new class. I added a test to catch this, and fixed the problem.
What’s interesting about this example is that I found the bug only because I was aggressively minimizing the non-private parts of my API. The less public API a class has, the fewer places there are for bugs to hide. The less public API there is, the easier it is for analyzers–static, dynamic, and human–to detect problems.
Many programmers subscribe to a cult of extensibility: Extensibility is always good. Opening up the API makes it more testable. Compile time static type checking really doesn’t help anyway. You might need it someday so put it in now. Maybe you don’t need it, but someone else might.
This way lies madness. Most extensibility points are never used. How many getters and setters are actually invoked? Not as many as you’d expect, and even fewer if test classes are not considered. How many non-final classes in your code actually have subclasses? How many of these classes have actually been designed and documented for extensibility? How many non-final fields actually are mutated after construction? Certainly some are. Not all methods and fields can or should be final. Some classes are designed to be subclassed. Some fields do need getters or less frequently setters. But none of these features should be added to your classes out of habit.
For maximum safety, remove as much as you can and lock down what you can’t. Make classes (and/or methods) final by default. Make fields final and immutable. Don’t mark a method public if it can be package protected instead. Don’t routinely add getters and setters for each field; and if you do need a getter or setter, just add the one you need, not both. In this case, I needed a setter, but the getter was gratuitous and removing it revealed a real bug.
Follow the YAGNI principle: You Ain’t Gonna Need It. Never add an extension or access point–be it a method, a field, a non-final class, etc.–until you know you need it. This will improve your code’s robustness, thread safety, security, speed, and more.
November 11th, 2008 at 1:16 pm
Python does not even have, as far as I know, access control, let alone private methods or final classes. Yet, in my experience, it scales very well in complexity.
The Linux kernel has wide open function calls and variables. You can easily crash and burn a Linux kernel by writing a faulty module, if that’s your intent. But people who write Linux kernel modules are not stupid (as a rule).
The key is to give hints to the developers. For example, if you have a getFlag method and a flag attribute, chances are that you are expecting people to call the getFlag method. No need to enforce it. If someone must use the flag attribute, they probably have a good reason. If they don’t and end up creating trouble for themselves, let them hang. It is their responsibility.
Ok. To be fair, I do mark some methods and attributes as private, but usually because they are ugly and I don’t expect anyone to ever understand how or when to call the method.
The overwhelming majority of bugs I create cannot be caught by static analysis. Everything you describe here relates to a very, very small minority of bugs that are usually very easy to catch early on.
Yeah. Ok. If you must publish a public API like XOM, then it makes sense to carefully mark things as private when you can. This makes the API easier to learn, if anything. However, very, very few people write public APIs. Most of us just hack relatively obscur code.
Also, of course, their are security issues, or even enterprise boundaries…. maybe the folks from one corporation need to publish an API they must support and so they must make sure that people don’t abuse it.
In short, what you state in an opinion, but it is not supported by my experience. It may be true in some cases, but they don’t constitute the overwhelming majority of code design.
November 13th, 2008 at 9:53 am
Daniel, that echos my experience as well. The problem with final is it’s finality 🙂 It does not admit the user might have a good reason to circumvent what the author (in his or her infinite foresight) was able to discern when writing the code. That sort of perfect foresight doesn’t exist and as a result there are many projects which are crippled by their inability to unit test code which uses those APIs.
People need the freedom to deal with local circumstances. An example: Superficially, it might seem like it would be a good idea to control cars so that they can never go beyond the speed limit. If you’re taking someone with a serious injury to the hospital, however, it no longer seems like a good idea.
November 13th, 2008 at 10:18 am
When your making a public API, it might be very annoying for the consumer when everything is made final. You can’t look in the mind of the programmers using your API. Over-finalization is bad. Keep the open-closed principle in mind: “Open for extension, but close for modification”. This doesn’t violate YAGNI.
Making things final when not needed but just to be sure does violate YAGNI.
November 13th, 2008 at 11:35 am
I follow YAGNI only when it is not extra work
If it is extra work to mark methods as private (esp like in the example get/setters), without perceivable gain – I am not game for it.
Same goes for final too. If It does not hurt to keep it open then why close it? esp. if u r not Nostradamus.
November 13th, 2008 at 9:24 pm
I think most of the posters come from
a framework background and mindset. Harold is talking about a library model. If you need extension points use interfaces. Subclassing correctly is hard, both from the Api creators standpoint and the consuming dev’s usage of the api.
If you do need to extend an api use composition and a facade
Pointing out large complex systems doesn’t invalidate rusty’s arguement. And his arguement applies to any language not just java
November 14th, 2008 at 3:39 am
I wholeheartedly agree with this philosophy when the project is run with total code ownership and anyone can modify classes, particularly because I think limiting the interface to a class or module allows you to more-easily reason about it. (Or maybe that’s just me?) For example, when I started working on a large Python codebase, everything was public, just as Daniel said. However, far too many bits of code all over the system were modifying internal state. This made refactoring needlessly difficult, so we’ve been migrating fields to __private. Now you can look at a class and have a pretty good assessment of how it’s used. If you need to use it somehow else, you can change it.
For public APIs, final and private should be used with caution.
November 14th, 2008 at 8:03 am
I’m not so sure I agree. I’ve worked with developers who aggressively finalize everything. Methods are package-private scope by default, only protected grudgingly, and public only when absolutely necessary. This leads to a lot of assumptions in the code that leads to nice subtle logic errors when the time comes to actually extend the code, or to use it in unforeseen ways.
Making a field non-final can invalidate a lot of rather hidden assumptions elsewhere in the code, especially when one is working with maps, where the immutability mean that it’s okay to use the object as a key rather than some aspect of the object. Make a field non-final, and change it, and really odd bugs start showing up … but since the class wasn’t ever thought of as a suitable key (it was just convenient), that works-as-a-key behavior won’t be documented or unit tested. etc. etc.
Instead of adopting a knee-jerk “policy” of minimizing the API, one should instead sit down and think about the API. One should make a list of the “minimal functional set” (everything you need to do with the data in this class, even if it’s a bit awkward), and then devise the “minimal useful set” (the minimal functional set with additional convenience
methods to avoid the awkward bits). Expose the minimal useful set and spend some time documenting and testing it… and then, as need be, expand the API with the bits of common code that make sense.
For example, if you’re defining an object that’s a collection of things, you’ll want a way to get at each of the things in the collection. That’s the minimal functional operation. It’s a good idea to be able to ask your object how many things it contains, but since you could figure that out by counting yourself, it’s not part of the minimal functional set, but it is arguably part of the minimal useful set of functions.
If the object is not immutable, then you’ll need a way to add new things, and where you add, you need to remove — even if nowhere in your code at the moment you’d need to remove anything. You could do this with a clear method, or with a remove method, as one or the other *could* be implemented using the other (plus the get-every-object method).
If you remove as much of the API as possible right now, then it’s likely that one developer will look at your class and say “Oh, it’s immutable, since there’s no way to change it.” and go off and write code that *depends* on that class being immutable. Meanwhile, a different developer will look at the code and say “This more-or-less does what I want, but it’s missing mutator methods, so I’ll just add those in.”… and go off and write code that *depends* on that class being mutable.
The compiler won’t complain. It’s unlikely that any unit tests will expose this bug until the two sets of assumptions are well and firmly entrenched.
Been there, done that, had to clean up the mess.
The “cult of extensibility” is just as bad, but in the other direction.
Somewhere in the middle is about right, with the minimally useful set of public methods, and private data. And I like sean’s point about interfaces… favor composition over subclassing. In fact, my inheritance trees are really shallow these days — I subclass for template methods, and very little else.
November 16th, 2008 at 9:31 am
Making classes and methods is absolutely bad idea anyway. Programmers need classes to be mocked, proxied etc. You cannot to do it with final classes.
November 16th, 2008 at 10:06 am
I meant
* Making classes and methods FINAL
November 24th, 2008 at 4:29 am
@Daniel: Python has something similar to access control but it is not enforced. So an attribute with a leading underscore is *meant* private but if you really want to you *can* use it (for e.g. tests). But if you use it in client code you know it may break in the future as it is *meant* private. So I guess it is something of both, ease of use and security for the API developer and user (if he wants it). (There are more strict possibilities to make an attribute something like private (two leading underscores or use of property() but the above should be sufficient for most needs.)
I guess for a language like Java using private the way defined above does make the most sense. Just to make something testable is not a sufficient reason as the trade-off is not good enough (as an API developer you are stuck with a non private field/method).
July 16th, 2009 at 11:09 pm
We always allow fully extendable classes. After 10 years of building software products with Java.
Times we had a bug from being open: 0
Times project almost failed due to a library with private and final classes: 20+
Keep things open! Final and private code is like black box programming.