I'm doing some heavy refactoring on a project that I've recently joined but that has been underway for months. The code is a mishmash of different styles and an uncertain architecture, so my task up to this point has been to make it more consistent, testable, and readable.
One of the issues I was tasked with was improving the data access layer. The system was using a pattern known as Repository for its data access layer, and the individual Repositories (the classes which both write data to and retrieve data from the database) are fairly dumb; they just do exactly what they say they do, without any validation or anything. Get, add, edit, delete, search; that's it.
The service interface layer of this app is an ASP.NET Web API application, so that app's Controllers need some way to get data from the database. In simple scenarios, the Controllers can just call the Repositories directly. However, the previous development team also included another layer of static classes between the Controllers and the Repository, called the Logic layer. The full architecture looked something like this:
Client | Controllers | Logic | Repositories | Database
I had a problem with the Logic layer. For the majority of the functionality the app handled, the Logic layer just called a Repository method and didn't do anything else. Further, it was a set of static classes, and therefore couldn't take advantage of the dependency injection scheme that I wanted to use in the Controllers. It seemed to me like an unnecessary layer of architecture, and so I removed it; the Controllers now called the Repositories directly.
Client | Controllers | Repositories | Database
Long time readers will know that this is par for the course for me: I'm an extreme deletionist and any code we keep must have a reason to exist.
On the surface, the architecture change makes sense. I'd been preaching to this group that controllers need to be small, repositories should only do data access, and less architecture is better than more. Seemed like a no-brainer to me. Little did I realize the no-brainer was me.
Did you catch the mistake I'd made? I'd waltzed into this project, taken over, and declared that I knew better than the two developers who had been working this project for six months prior. I had (unintentionally, to be sure) declared that my opinion on how this app should be constructed was more important than theirs, simply because it had worked for me in the past. I had given myself a golden hammer, and now everything looked like a nail.
I, of course, didn't notice at all. The other two did. For this post we'll call them Rajit and Dave. Here's how that conversation went:
Dave: Hey Matt, did you remove the Logic classes that Rajit and I set up?
Matthew: Sure did! I couldn't see that they were doing anything different than if the controllers called the Repository classes directly. So I moved all that code to the Controllers. Now we don't need anything between Controllers and Repositories. It's a simpler design overall.
Rajit (stunned): ...Um, OK, it is simpler, kind of. But remember, this API will need to call other APIs to get all the data we need to send back to the requesting application. Where would it do that?
Matthew (realizing the mistake, not wanting to admit it): Well, wouldn't it do that in the Controllers....?
Dave (annoyed): But didn't you say that we want to keep the controllers small? Why bloat them like this when we could do the extra, non-database-access code in a different layer then just call that from the controller? Or are we not worrying about bloat on the controllers? Which is it?
It's easy to fall into the trap of "I've seen this before and X worked, so I'll just do X again". Programmers (including myself) are busy people, often over-worked, and don't necessarily have time to consider all possible solutions to a given problem. Sometimes, the lack of time (or foresight, as is the case here) comes back to bite us in a big way.
Dave and Rajit were right, of course: we ought to have that extra layer to do things like consolidate calls from other web services and make multiple Repository submissions. I hadn't immediately found the Logic layer's reason to exist and had removed it, when in reality there was a purpose for this layer; I had just missed it. I still objected to the Logic layer being a set of static classes, so eventually Dave, Rajit, and I compromised to include a new set of Service classes:
Client | Controllers | Services | Repositories | Database
The Services could call the Repositories and any other services, and because they were no longer static, we could inject them into the Controllers. I got my simpler architecture, and Rajit and Dave got their extra layer. Everyone's happy.
Point is, don't just assume that every app needs the same architecture. There are a myriad of software patterns for this reason: each of them has a different purpose, a different use case. They aren't all interchangeable. Don't assume the solution to a given problem is the same as a similar problem you've encountered before. Similarity does not mean sameness.
(Also, it helps if you actually talk to people who have more experience than you on a given project. Just sayin'.)
Anybody screwed up like this before? Alternately, any of you lovely readers find an over-architected app and pare it down successfully? Was there a better pattern for this app that Rajit, Dave, and I could've used? Share in the comments!