A Misadventure in (someone else’s) GoF Land
So, here I go again. I don’t want my sparsely populated blog to turn into a collection of articles from the Daily WTF, but I do want to use a real world example, with the names changed to protect the innocent and guilty, to illustrate a point.
Design patterns, object oriented design, and the employment thereof can be just as much a cure in search of a disease as they are a cure for a disease. I’ve recently run into this lock, stock, and barrel in a project with my unnamed employer.
There is this project, let’s call it FLEA, which is a relatively straightforward web forms application. Written as a front end to handle some day to day process for a particular customer, it was made out to be some kind of shimmering example of, well what the architect might have said “Stuff I read on the back cover of a book in the lavatory.”
It had “everything” from service locators, coding against interfaces, and perhaps most glaringly, a monstrous construct that was referred to as “The Repository Pattern on steroids.”
It’s one thing to permit reuse of code by decoupling interface from implementation, but it’s quite another to hobble the developer by only making available interfaces that operate on fully populated domain objects.
I saw places where they would set a flag in a table record by loading an object, all of it’s sub-objects, and sub-objects, with carefully constructed IF-THEN logic to subvert graph loops, then set ‘myPerson.Disabled = true; PersonRepository.Save(myPerson);’. Now its fine if you present that interface, then detect that only the Disabled flag changed, and update just that, but what I found was quite another beast: It proceeded to delete half of the sub-objects, re-add them, then ignore the rest of them, and do a multi-table update of the full object (without using a join, anywhere in the logic) creating a new database connection for every other call. Needless to say, this worked brilliantly when the thing only loaded 2 or 3 master objects but when it was scaled to include ten thousand users, disabling a user, and I use this phrase QUITE LITERALLY sent the server out to lunch. You could go to the corner store and back before it finished running a top-down depth loop of connection grabbing calls to update stuff.
It’s still not probably quite clear the degree of repository extremism I’m talking about here:
We had:
PersonRepository (with methods to GetAll, GetByLocation, GetById, Update, and Delete)
PersonLocationRepository (which was called repeatedly for every person loaded, even when displaying them in a dialog box, to avoid having to learn what a join is.)
PersonLocationActivityRepository
ActivityRepository
PlacesRepository
PersonEthnicityRepositiry
PersonJobCodeRepository
PersonHRRecordRepository
PersonMotherInLawRepository (Ok, I’m making that up.)
plus about 20 more (not exaggerating, I swear.)
That variety alone is not a problem. But calling LoadPerson or whatever the actual routine was called invoked a service locator, which loaded a bunch of other things, which called a service layer, which called a business logic layer, which then loaded a repository, which loaded a DAL object for each one of these things, DEEP. So that loading a person ended up instantiating a generic service loader factory, which created a service layer factory, and then proceeded to call a business logic layer factory, to create a DAL factory, and eventually instantiate a DAL object, to read the ethinicity of a person. Then all of those were destroyed, and the PersonHrRecord was loaded with a new chain of the same stuff.
Loading the user administration page literally kicked off 300,000 database connections, and called half a million stored procedures, loaded ALL of the data client side, and filtered out the results with fancy anonymous method calls to IList enumerators. And just because they realized what a hulking mess this was on the database, all of the results were chucked into ViewState, which to save time was chucked into the Session.
I, along with two others, retooled the entire project using Linq To SQL in about a month of half time working on this, but wow it was amazing.
If it would have been a well crafted joke, it would have been too meticulously executed to be even considered funny.
My conclusion is that it’s actually bad because of the way they made the database calls. Not the patterns themselves. There was also a demonstrable lack of understanding of what the implications of iteratively looping and loading are for performance.
The theory, I was told, was to make it reusable. From a design perspective, it looked like they might have been able to do that, but when I looked at what the actual layers were doing, apart from what they looked like in UML diagrams, MAN was it a mess.
Put another way, it’s one thing to design your application for reusablity, but quite another to implement it in the same spirit. At the outset of the project, it probably was originally, long ago, in a galaxy far away, a good idea to design a service layer separate from business logic layer, and a DAL layer, etc. But to just stop there and make the rest ‘work somehow’ without concern for the consequences of the ways the data actually got into the bottom bits.. and the way that it was unfiltered until the top bits, that was just silly. Of course I shouldn’t blame the pattern, but if they hadn’t been aiming for that methodology, as opposed to the operations that they were actually performing underneath it, it might have been a better ending.
Sure it’s common to say ‘That’s just an implementation detail’ when you’re designing, and scoff it at for a while until it comes time to implement it, but holy crap, when it comes time to implement it, all you HAVE are the implementation details. At that point they not only deserve SOME attention, but ALL of it.
This is the difficulty with GoF: Pretty Conceptual Pictures. People think they understand these, and it gives them the confidence to go implement this stuff without learning the other fundamentals. Like, well, SQL. (I’ve actually heard things like “I’m a C# developer, I don’t really DO SQL!” If I had my way, I’d put them in a corner with a command line OSQL.exe tool for the next three months and not let them come back until they’d gone through Books Online from top to bottom three times after that statement.) Pretty pictures like this also make it easy to gloss over an actual lack of planning on the bottom so that management doesn’t know they’ve bought into a dud until the delivered app kills the clients database farm.
The goal of an application is to have it work. Whatever abstractions you put on it to make it ‘easier to develop’ are not billable features to the client. You do not get extra line items on the proposal for Service Locator Patterns, and certainly that’s not a justification for screwing up the actual project.
Customer’s Boss: “This thing is a piece of crap, it kills our servers!”
Developer: “But it was completely decoupled from the implementation!”
Customer’s Boss: “Oh, right, sorry, I guess you did actually deliver what we ordered. I’m glad you know what you’re doing when it comes to object oriented design. So many people focus solely on the final product, without so much as a thought devoted to the design process. Clearly you have attended one of our finest higher educational institutions to know such things. And besides, I didn’t realize you were a SENIOR developer, I should be yelling at a grunt code right now. You shouldn’t be troubled with implementation details.”
Developers Boss: “I smell a RAISE for one of our brightest SENIOR developers!”
Developer: Nancy Kerrigan and I are going to Disney Land!
