46

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?

10
  • 4
    =~ s/necessary/recommended/gi Dec 4, 2013 at 20:50
  • 2
    Data types should be correct by construction, or else what are you building on? They should be encapsulated in such a way that, mutable or not, they can only ever be in valid states. Only if it’s impossible to enforce this statically (or unreasonably difficult) should you raise a runtime error.
    – Jon Purdy
    Dec 5, 2013 at 9:34
  • 1
    Never say never. Unless your code is never used, you can never know for certain where your code will end up. ;)
    – Izkata
    Dec 5, 2013 at 14:11
  • 1
    @codebreaker GrandmasterB's comment it's a replace expression. It means: replace "necessary" with "recommended". Dec 8, 2013 at 14:21
  • 1
    The Codeless Code #116 Trust No One is likely particularly appropriate here.
    – user40980
    Mar 14, 2014 at 23:55

6 Answers 6

72

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.

2
  • Good to know. In the past I used to just program as efficiently as I could, so I sometimes have a tough time getting used to ideas like this. I'm glad I was going in the right direction. Dec 4, 2013 at 20:57
  • 15
    "Efficiently" can mean many different things! In my experience, novices (not that I'm saying you are one) often overlook how efficiently they will be able to support the program. Code usually spends far longer in the support phase of its product life cycle than it does in the "writing new code" phase, so I think that is an efficiency that should be considered carefully. Dec 4, 2013 at 21:08
26

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.

2
  • 1
    +1 for mentioning Design by Contract. If you can't completely prohibit behavior (and that's hard to do) at least you make clear that there are no guarantees on bad behavior. I also like throwing an IllegalStateException or an UnsupportedOperationException.
    – user949300
    Dec 4, 2013 at 22:03
  • @user949300 Sure. I like to believe such exceptions were introduced with a meaningful purpose. Honoring contracts seems to fit such role.
    – afsantos
    Dec 4, 2013 at 22:34
16

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.

2
  • 2
    +1 for "Until it starts getting in the way of writing code." Especially for short-run personal projects, coding defensively can take far more time than it's worth.
    – Corey
    Dec 5, 2013 at 3:59
  • 2
    Agree, though I would like to add that it's a good thing to be /able/ to program defensively, but it's also crucial to be able to program in a prototyping fashion. The ability to do both will allow you to choose the most appropriate action which is far better than a lot of programmers I know who are able to only program (sort of) defensively. Dec 5, 2013 at 14:37
13

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.)

2
  • 1
    I considered the zone being a property of the card, but since my Cards work better as immutable objects, I decided this way was best. Thanks for the advice. Dec 4, 2013 at 20:40
  • 3
    @codebreaker one thing that can help in that case is encapsulating the card in another object. An Ace of Spades is what it is. Location does not define its identity, and a card probably should be immutable. Maybe have a Zone contain cards: maybe have a CardDescriptor that contains a card, its location, face up/down status, or even rotation for games that care about that. Those are all mutable properties that do not alter a card's identity.
    – user22815
    Mar 7, 2014 at 21:33
1

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.

3
  • 2
    Your answer focuses a bit too much on the problem at hand instead of the broader questions the OP is asking about defensive programming in general.
    – user53019
    Dec 4, 2013 at 20:35
  • I actually do pass the Zones to methods that take Collections, so the implementation is necessary. A sort of registry of zones within the game is an interesting idea, though. Dec 4, 2013 at 20:46
  • @GlenH7: I find working with specific examples often helps more than abstract theory. The OP provided a rather interesting one, so I went with that. Dec 5, 2013 at 14:11
1

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.

Not the answer you're looking for? Browse other questions tagged or ask your own question.