How necessary is it to follow defensive programming practices for code that will never be made publicly available?

Question

I'm writing a Java implementation of a card game, so I created a special type of Collection I'm calling a Zone. All modification methods of Java's Collection are unsupported, but there's a method in the Zone API, move(Zone, Card), which moves a Card from the given Zone to itself (accomplished by package-private techniques). This way, I can ensure that no cards are taken out of a zone and simply vanish; they can only be moved to another zone.

My question is, how necessary is this kind of defensive coding? It's "correct," and it feels like the right practice, but it's not like the Zone API is ever going to be part of some public library. It's just for me, so it's kind of like I'm protecting my code from myself when I could probably be more efficient by just using standard Collections.

How far should I take this Zone idea? Can anyone give me some advice on how much I should think about preserving the contracts in classes I write, especially for ones that aren't really going to be publicly available?

Answer 1

I'm not going to address the design problem - just the question of whether to do things "correctly" in a non-public API.

it's just for me, so it's kind of like I'm protecting my own code from myself

That's exactly the point. Maybe there's coders out there who remember the nuances of every class and method they ever wrote and never mistakenly call into them with the wrong contract. I'm not one of them. I often forget how code I wrote is supposed to work within hours of writing it. After you think you've gotten it right once, your mind will tend to switch gears to the problem you're working on now.

You have tools to combat that. These tools include (in no particular order) conventions, unit tests and other automated tests, precondition checking, and documentation. I myself have found unit tests to be invaluable because they both force you to think about how your contract will be used and provide documentation later on how the interface was designed.

Answer 2

I usually follow some simple rules:

• Try to always program by contract.
• If a method is publicly available or receives input from the outside world, enforce some defensive measures (e.g. IllegalArgumentException).
• For everything else that is only accessible internally, use assertions (e.g. assert input != null).

If a client is really into it, they will always find a way to make your code misbehave. They can always do it through reflection, at least. But that's the beauty of design by contract. You don't approve such use of your code, and so you can't guarantee that it will function in such scenarios.

As to your specific case, if Zone isn't supposed to be used and/or accessed by outsiders, either make the class package-private (and possibly final), or preferably, use the collections Java already provides you. They're tested, and you don't have to reinvent the wheel. Notice that this doesn't prevent you from using assertions throughout your code to make sure everything works as expected.

Answer 3

Defensive programming is a very good thing.
Until it starts getting in the way of writing code. Then it's not such a good thing.

---

Speaking a bit more pragmatically...

It sounds like you're right at the edge of taking things too far. The challenge (and the answer to your question) lies in understanding what the business rules or requirements of the program are.

Using your card game API as an example, there are some environments where everything that can be done to prevent cheating is critical. Large amounts of real money may be involved, so it makes sense to put a large number of checks in place to make sure that cheating can't occur.

On the other hand, you need to remain mindful of the SOLID principles, especially single responsibility. Asking the container class to effectively audit where cards are going may be a bit much. It may be better to have an audit / controller layer between the card container and the function that receives the move requests.

---

Related to those concerns, you need to understand what components of your API are publicly exposed (and thus vulnerable) versus what is private and less exposed. I'm not a total advocate of a "hard exterior coating with a soft inside", but the best return of your effort is to harden the exterior of your API.

I don't think the intended end user of a library is as critical of a determination about how much defensive programming you put in place. Even with modules that I write for my own use, I still put a measure of checking in place to make sure that future me didn't make some inadvertent mistake in calling the library.

Answer 4

Defensive coding is not just a good idea for public code. It's a great idea for any code that is not immediately thrown away. Sure, you know how it's supposed to be called now, but you have no idea how well you'll remember this six months from now when you come back to the project.

Java's basic syntax gives you a lot of baked-in defense compared to a lower-level or interpreted language like C or Javascript respectively. Assuming that you name your methods clearly and don't have external "method sequencing", you can probably get away with simply specifying arguments as a correct data type and including sensible behavior if properly-typed data can still be invalid.

(On an aside, if Cards always have to be in zone's, I think you get better bang-for-the-buck by having all cards in play be referenced by a collection global to your Game object, and have Zone be a property of each card. But since I don't know what your Zones do other than holding cards, it's hard to know whether that's appropriate.)

Answer 5

First create a class that keeps a list of Zones so you don't lose a Zone or the cards in it. You can then check that a transfer is within your ZoneList. This class will probably be a sort of singleton, as you'll only need one instance, but you might want sets of Zones later, so keep your options open.

Second, don't have Zone or ZoneList implement Collection or anything else unless you expect to need it. That is, if a Zone or ZoneList will be passed to something that expects a Collection, then implement it. You can disable a bunch of methods by having them throw an exception (UnimplementedException, or something like that) or by having them simply do nothing. (Think real hard before using the second option. If you do it because it's easy you'll find you're missing bugs you could have caught early on.)

There are real questions about what is "correct". But once you figure out what it is you'll want to do things that way. In two years you'll have forgotten about all this, and if you try to use the code then you'll get real annoyed at the guy who wrote it in such a counterintuitive manner and didn't explain anything.

Answer 6

Defensive coding in API design generally is about validating input and carefully selecting a proper error handling mechanism. Things other answers mention are also worth noting.

This is actually not what your example is about. You are there limiting your API surface, for a very specific reason. As GlenH7 mentions, when the set of cards is to be used in an actual game, with a ('used' and 'unused') deck, a table and hands for example, you definitely want to put checks in place to make sure each card from the set is present once and only once.

That you designed this with "zones", is an arbitrary choice. Depending on the implementation (a zone can only be a hand, a deck or a table in above example) it might very well be a thorough design.

However, that implementation sounds like an derived type of a more Collection<Card>-like set of cards, with a less restrictive API. For example when you want to build a hand value calculator, or an AI, you surely want to be free to choose which and how many of each cards you iterate over.

So it is good to expose such a restrictive API, if the only goal of that API is to make sure each card always is in a zone.