Feed Sign in with OpenID OpenID

Simon Willison’s Weblog

How not to use OOP

Via Hans Nowak, Understanding Object Oriented Programming, or how to turn 19 lines of easily maintained code in to an OO monstrosity spanning 7 class files. This is not the way to make code more maintainable. For comparison, here’s how I would implement a solution to the same problem in Python, assuming the availability of an equivalent function to Java’s System.getProperty("os.name") (os.name is similar but inappropriate for this example):

MESSAGES = (
  ('SunOS', 'Linux'), 'This is a UNIX box and therefore good.'),
  ('Windows NT', Windows 95'), 'This is a Windows box and therefore bad.),
)
DEFAULT_MESSAGE = 'This is not a box.'

def get_os():
    name = System.getProperty("os.name")
    for names, message in MESSAGES:
        if name in names:
            return message
    return DEFAULT_MESSAGE

if __name__ == "__main__":
    print get_os()

In my book, smart data-driven programming beats over-engineered class-based programming any day of the week.

Posted 5th December 2003 at 11:10 pm

This is How not to use OOP by Simon Willison, posted on 5th December 2003.

View blog reactions

Next: Hacked for Spam

Previous: Bounty Hunting

19 comments

  1. Yes! _Data driven_ programming!

    Keith - 5th December 2003 23:53 - #

  2. The problem with examples is that to get a point across, you have to make the problem simple. And a simple problem doesn't need OOP.

    So it's hard to make a good OOP example.

    Jeremy Dunck - 6th December 2003 00:15 - #

  3. I agree completely, but you've got to admit that isn't just not a good OOP example, it's actively a terrible one. I love OOP - heck, I work with Python every day which is about as object oriented as they come (although Ruby and Smalltalk advocates might disagree). However, you can definitely take a good thing too far. If your aim is easily maintainable code there's a lot to be said for a data-driven approach to problem solving.

    Simon Willison - 6th December 2003 03:28 - #

  4. Yes, I agree that your suggested solution is quite a bit more understandable.

    This might seem a silly question, but-- can you suggest any reading on the data-driven approach?

    Jeremy Dunck - 6th December 2003 05:58 - #

  5. Footnotes: 1) I'd use a dictionary instead, and reduce the get_os() function to "return MESSAGES.get(sys.platform, DEFAULT_MESSAGE)". 2) sys.platform is good enough for this case; if you need more granularity, use platform.platform(). 3) if anyone really needs to implement something like this, "sys.platform == "posix" is an even better choice: it'll do the right thing also for "good platforms" that the original developer didn't know about...

    Fredrik Lundh - 6th December 2003 10:06 - #

  6. Better make that: 3) if anyone really needs to implement something like this, os.name == "posix" is an even better choice: it'll do the right thing also for "good platforms" that the original developer didn't know about...

    Fredrik Lundh - 6th December 2003 17:31 - #

  7. "In my book, smart data-driven programming beats over-engineered class-based programming any day of the week." What book?

    Ron Green - 6th December 2003 21:45 - #

  8. Fascinating example.

    What I don't like about the OO solution though is it creates an instance of each of the BoxSpecifier implementations - any given user running the APP will only have one OS installed so why carry around instances of classes representing other OS's? As it support's more OS's, more memory gets eaten up with those singletons and searching the Hashmap is going to take longer and longer. This may be premature optimization but optimizing later with that design may involve a major re-write. Otherwise, each BoxSpecifier is "bound" to the OSDiscriminator via the register method. That may be a problem should there be a need to use them seperately from OSDescriminator.

    That said, it does have the advantage that each class implementing BoxSpecifier can be unit tested. Updates to the MESSAGES tuple may be more prone to human error as it grows.

    Perhaps another approach would be something between the two. MESSAGES simply maps OS names to the name of a class to create an instance of, from which the message can be retrieved. The class can be unit tested. MESSAGES could stored persistently and only examined for the first call to get_os() (at which point an instance of the "boxspecifier" is created and further calls to get_os() use that instance, rather that needing MESSAGES again.

    Harry Fuecks - 6th December 2003 23:30 - #

  9. I went for a tuple structure rather than a dictionary because a dictionary would end up having duplicated information - the keys 'SunOS' and 'Linux' would both have the same value, so if the value was changed in one place it would need to be changed in the other as well.

    Simon Willison - 7th December 2003 04:48 - #

  10. "... each class implementing BoxSpecifier can be unit tested."

    If your test framework doesn't support unit testing of things that are not classes, you need a better test framework.

    "if the value was changed in one place it would need to be changed in the other as well."

    Sure. But don't tell me you don't know how to handle that? ;-)

    Fredrik Lundh - 7th December 2003 08:57 - #

  11. "If your test framework doesn't support unit testing of things that are not classes, you need a better test framework."

    True but with MESSAGES, it's going to be difficult to test the units. Testing the structure should be no problem but testing that "Windows CE" really corresponds to "This is a Windows box and therefore bad." will requiring parsing all of MESSAGES. Do you use get_os() to do this for you are reimplement it in a test?

    I'm still on the lookup out for ideas on how to unit test parsers...

    Harry Fuecks - 7th December 2003 12:00 - #

  12. the get_os function is the unit, right? just let the test framework (temporarily) override sys.platform (or platform.platform) during testing, and you're done.

    (or are we talking about two different things?)

    Fredrik Lundh - 7th December 2003 12:21 - #

  13. "(or are we talking about two different things?)"

    Think so. This example, as Jeremy mentioned, is kind of trivial so doesn't really highlight the need for testing but what I'm getting at is I think MESSAGES itself is a collection of "units".

    For example think the relationship between 'Windows 95' and 'This is a Windows box and therefore bad.' is a single unit (easier to see in the OO example). Ideally it should be possible to test each of these, independently from the collection of MESSAGES and independently from from get_os(), otherwise you're not sure you're really testing what you wanted to test.

    Really this example needs some further complexity thrown in - how about i18n? Needs to be possible to display the returned messages in other languages. Come to think of it, along the same lines but more relevant would be a browser sniffer library, able to report JavaScript implementation etc. etc.

    Harry Fuecks - 7th December 2003 14:23 - #

  14. I've learned painfully and slowly that trying to account for flexibility in all directions makes really, really complex code.

    If the code doesn't need to be flexible in a certain direction, making it so adds needless complexity.

    Their OOP example is easily seen as overblown because it accounts for all kinds of flexibility that's not really needed for the stated problem.

    I'm sure that I could come up with a monkey wrench that their ivory-tower design does not easily address.

    Harry, your suggestion of a i18n feature will require flexibility in a different direction, and will justify some more complexity. But it doesn't change the fact that their recommended-design is needlessly complex.

    I like simple code. I hate duplication. I like unit tests. I like refactoring. It works out for me. :)

    Jeremy Dunck - 7th December 2003 23:01 - #

  15. This is also a good place where having domain knowledge comes in handy. The number of operating systems is small and doesn't grow very quickly. The need for a "factory" to handle frequent adds clearly doesn't make sense here, and the chance of the hacker solution becoming unwieldy is small. Maybe it's my embedded background, but code overkill is something to be avoided whenever you can.

    Tom Maszerowski - 8th December 2003 19:01 - #

  16. That link's url's last fragment reads out loud as "pee poop", which kind of sums up what I think of the "OOP for everything, even though we don't need it, nor will we ever" approach.

    Jesper - 9th December 2003 20:48 - #

  17. Shouldn't there be a closing apostrophe in the "This is a Windows..." string?

    Jeff Walden - 10th December 2003 19:51 - #

  18. What I take from this example is that the key distinction between a true religious zealot and the rest of us poor slobs is the lack of a sense of humor. This isn't just an actively terrible OOP example, it's a HILARIOUS example. It's as if Monty Python wrote a skit on OOP! (I see John Cleese becoming progressively more crazed as he explains to a nervous victim why each new version is more whizzo-bango than the last). In fact, isn't it possible that this is actually a satire?!? I mean, how did the author keep from cracking up as he wrote it?

    Which brings us to the real question. If it's truly a fine line between the sublime and the ridiculous, how can we keep on the right side of it? Personally, I think the road to hell equals good intentions whose goodness is unquestioned. So the antidote is to give in to your evil desire to punch holes in all that goodness, aka humor. The proper response to the last OOP example is to run around the room shouting DANGER! DANGER! WILL ROBINSON! like a robot.

    In this vein here are a few more instructive lessons on the dangers of logic taken to its logical conclusion: satire.

    http://www.teemings.com/extras/truelife/scylla6.ht ml

    http://www.wackywarnings.com

    Christopher Lee - 7th March 2004 21:06 - #

  19. This ver is shorter and simpler, IMO. msg_unix = 'This is a UNIX box and therefore good.' msg_win = 'This is a Windows box and therefore bad.' msg_map = {'SunOS': msg_unix, 'Linux': msg_unix, 'Windows NT': msg_win, 'Windows 95': msg_win} def get_os(): return msg_map[System.getProperty('os.name')] if __name__ == "__main__": print get_os() Note that I raise an error (KeyError) when the OS is unknown. If you prefer a default message, use msg_map.get(System.getProperty('os.name'), msg_def).

    Connelly Barnes - 19th June 2004 20:59 - #

Comments are closed.

Previously hosted at http://simon.incutio.com/archive/2003/12/05/dataDriven

A django site