Skip to content

The Good Citizen

March 16, 2009

In the comment at the bottom of a previous blog post, I gave a link to The Good Citizen. It turns out that Rob never saw it, and given he’s one-third of my readership, and if anything it’s much more worth reading than anything I write, I thought it worth dedicating a whole post to it. Just in case that link every goes bad, here it is in full…

As a good citizen, I…

  • Keep a consistent state at all times – init() or populate() is a code smell.
  • Have no static fields or methods
  • Never expect or return null.
  • Fail-Fast – even when constructing.
  • Am Easy to test- all dependent object I use can be passed to me, often in my constructor (typically as Mock Objects).
  • Accept dependent object that can easily be substituted with Mock Objects (I don’t use Concrete Class Dependency).
  • Chain multiple constructors to a common place (using this(…)).
  • Always define hashCode() alongside equals()
  • Prefer immutable value objects that I can easily throw away.
  • Have a special value for ‘nothing’ – e.g. Collections.EMPTY_SET.
  • Raise checked exceptions when the caller asked for something unreasonable – e.g. open a non-existent file.
  • Raise unchecked exceptions when I can’t do something reasonable that the caller asked of me – e.g. disk error when reading from an opened file.
  • Only catch exceptions that can be handled fully.
  • Only log information that someone needs to see.

Classes that are designed for Constructor Injection are better citizens than those that are not.

I agree with pretty much all of that, and some pointsare particularly interesting and/or useful. For example, the two points about exceptions are, I think, a really helpful guideline to be used when deciding if and what to throw. Anyway, I’ve only just back from Agile School and I’ll be in trouble if I stay up any later, so I’ll post this and follow it up in the comments…

Advertisements
8 Comments leave one →
  1. dan permalink
    March 16, 2009 11:05 pm

    Oh, but before I forget, remind me to ask Matt about mocking concrete (non-interface) classes…

  2. March 17, 2009 12:59 pm

    In my head, I’ve always decided whether to throw checked or unchecked exceptions based on whether it was “recoverable” but I think that “reasonable” may be slightly better.

    The exception example you have above is interesting. Let’s fire up a discussion:

    1. Why do you think I should throw a checked exception if the file does not exist but unchecked if there is a permissions error?
    2. Are you saying the caller should assert that file.exists() but doesn’t have to assert file.canRead()?
    3. Are you saying that canRead() is a special case because another process could lock the file etc? Could another process not delete that file?
    4. Why not throw unchecked exception for everything and the caller only catches what it can handle?
    5. Are checked exceptions for errors you expect, and therefore, not “exceptional” behavior at all?
    6. In Java, is declaring that you throw a runtime exception pointless?
  3. dan permalink
    March 17, 2009 1:47 pm

    well, for a start, *I* didn’t say it, I just stole it from the article *you* showed me 😉

    I particularly like the guidelines for checked/unchecked exceptions simply because no-one else has ever given me any, and everything else in the article is good sense, so that should be too, right?

    You didn’t number your points, so I edited your comment and changed it to an ordered list. As you can see, it didn’t work… It doesn’t matter, because I’m going to ignore most of them anyway.

    Let’s think about the client for our exceptional file-opening-and-reading class (FOARC). you seem to be saying something like:


    void doFoarc {
    assert(myFile.exists());
    myFoarc.doStuff(myFile);
    }

    whereas I read it as:


    void doFoarc {
    try {
    myFoarc.doStuff(myFile);
    }
    catch (FileDoesntExistException e) {
    // throw or recover, depending on what's appropriate
    }
    // there isn't any appropriate response to FileReadError,
    // so it's not caught, nor do we have to do so
    }

    Or something like that. Anyway, I gotta go.

  4. March 17, 2009 3:30 pm

    Yes, you’re right, I misread the thingy.

    There’s a discussion of this on stackoverflow (there’s no preview so apologies if that looks horrible). Including some comments to say that checked exceptions were a mistake (because unchecked exceptions “fail-fast”) and quite a funny one where someone says “I patched javac to make all exceptions unchecked.” (which has been marked down by the other users somewhat unfairly I think, but then he didn’t really try to defend himself).

    Saying you like the guidelines because no-one else gave you any is probably the best possible reason for liking them. In my first job from uni a boss at the time asked me what my opinion was on this exact topic in an email. At the time I assumed he valued my opinion on such matters. In hindsight, he probaly realised I didn’t even know there was a difference and he knew asking me in email would make me go off, learn something, and pretend I already knew it.

  5. dan permalink
    March 17, 2009 11:29 pm

    Going back to some of your points that I missed out first time round…

    Are checked exceptions for errors you expect, and therefore, not “exceptional” behaviour at all?

    Pete once told me that he’d read an excellent book on writing code with exceptions, and that it had really helped him with his code. One day, I’ll ask him what it was (though I’m still working my way through my Rob Allen reading list). Anyway, maybe you’re right; maybe we need need the names to be more different than Checked and Unchecked. And maybe there’s a “reverse assert” design pattern, where, instead of asserting things on the way in, you catch asserts on the way out. I mean, what’s the point of checking that a file exists on the way in when the read operation implicitly does so? Don’t Repeat Yourself, just catch the assertion at the same level that you’d logically do the assert.

    There’s more, but I’ve run out of time. Maniana…

  6. March 18, 2009 11:28 am

    Suppose you were invoking a method on a 3rd party API for which you didn’t have the source code. The method you wanted to invoke was:

    public int getThirdFactor(int input) throws InputNotPrimeException;

    Where InputNotPrimeException is a checked exception (sorry I can’t think of a sensible example!). If you had a number and you wanted its 3rd factor but you didn’t know if it was prime what would you do?

    You could just give it the number and trust the method to let you know if it was prime. Hopefully the method “fails-fast”. You are passing a method input that you know may be invalid. Are you being a good citizen?

    You could check if it’s prime before invocation, but that doesn’t sound very DRY. This is a silly example but could still have big performance implications.

    For the code to be cohesive, finding out if a number is prime, and finding out the 3rd multiple are seperate tasks. Maybe that method calls a isPrime(int n) method at the start to validate its input, but implementation details are none of our business, we don’t want to fish around in its undies.

    A Defensive Programmer would ensure all input is validated with paranoia and trust no-one.

    Another way would be to pass data structures that themselves are good citizens (and thus have consistent and legal state) at all times. In this case we could have a NonPrimeInteger which would throw a NonPrimeException on construction. The method would not need to validate input for non-primeyness then because that would be guaranteed.

  7. dan permalink
    March 18, 2009 7:03 pm

    Good example; or should I say, examples, as you drift from using an API to writing it. (Yes, the italics probably are overkill, but I want to try out my new buttons).

    Let’s start with the easy one – Defensive Programming (DP) is bad. Don’t do it. Why? Because somebody told me so. But I could (and probably should) dedicate a whole lunchtime talk to it (despite not having any formal qualification to do so). Where should I begin?

    First of all, it leaks everywhere. People start being defensive, and don’t know when to stop. Second of all, it just adds credence to the preposterous notion of the ghost in the machine; that computer programs are inheritantly complex, stocastic and irrational, and that they will always be buggy and difficult to compehend, fix and maintain. Just check out the opening lines from the wikipedia entry on DP:

    Defensive programming is a form of defensive design intended to ensure the continuing function of a piece of software in spite of unforeseeable usage of said software. The idea can be viewed as reducing or eliminating the prospect of Murphy’s Law having effect.

    Murphy’s Law? Seriously, what next? All programmers to be equipped with horseshoes and rabbit’s feet?

    The positives of DP aren’t DP at all, they’re just good programming. You should always have an adapter between your 3rd-party library  or xternal input and the rest of your code, that does much more that just ensuring input strings are null-terminated. What happens when you find a bug in the library? What happens when you find so many bugs in the library, that you need to swap it for another one? Your internal adapter is the place where all such checks and changes happen. Your internal adapter is where you take NonPrimeIntegers and pass them onto the int-based getThirdFactor(). Your internal adapter is where you program to an interface, allowing all of your calling code to be tested without invoking the API.

    So, yes, if you’re writing the API, use objects to enforce each of the preconditions; and if you’re using the API, write your own layer to make it happen.

    There we go. I think that’s sufficiently militant.

  8. dan permalink
    March 18, 2009 7:18 pm

    Going back to my earlier example of…

    void doFoarc() {
    try {
    myFoarc.doStuff(myFile);
    }
    catch (FileDoesntExistException e) {
    // throw or recover, depending on what's appropriate
    }
    // there isn't any appropriate response to
    // FileReadError,
    // so it's not caught, nor do we have to do so
    }

    …the point I was going to make was that an important part of the art of exception-handling, is catching the exception at the correct level (or so I’m told, anyway). In the file-reading example, let’s imagine that you’ve some GUI code prompting the user for a file, and that the input is passed down a few layers, eventually finding doFoarc(). Where do you catch the exception if and when it’s thrown back up?

    I think that a lot of programmers (myself included) are scared of letting exceptions go too far, and will catch the exception and try to feed back some error-code return value, or something equally vomit-inducing.

    No!

    Let the exception get all the way up to the GUI code, where you can write something like…

    okOrCancel doFoarcGUI() {
    myFile = showFileDialog();
    try {
    return doFoarc(myFile);
    }
    catch (FileDoesntExistException e) {
    showModalErrorMsgBox("File Doesn't Exist", MBOK);
    return doFoarcGUI();
    }
    }

    Go recursive programming! Sure, my code is tripe and full of holes, but you get the idea. Pass the exception/assertion right up to the layer that can handle it best.

    I’m sure that someone else has written something else much more elegant to demonstrate my point. Probably in that book Pete was talking about. I wonder where I put his email address…

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: