Design Review and the Single Responsibility Principle

Back when I first started managing a team of developers one of the things I instituted was code reviews. We created some coding standards and a list of best practices to follow including naming conventions, error handling, and optimizations around performance and we inserted a review step into our development process. After a while writing code that followed our standards became second nature to the team so we would go into the reviews, validate the code, and wrap up the meeting in a very short period of time. Where the process fell short was we were focusing on the code, not the design. Writing good, clean, uniform, and optimized code is great but what is probably more important is to have a good design.

A good design has a lot of characteristics but one of the most important signs of a good design is a solid breakdown of class responsibilities. The Single-Responsibility Principle states that a class should have only one reason to change. (“Agile Principles, Patterns, And Practices in C#” Robert C Martin and Micah Martin).

Here is an example requirement:

When a file is dropped into a folder, attach some meta-data to it and move it to a secure location (say a file stream).

From a high level there are a few things we need to do

• Monitor a folder looking for new files.
• Create meta-data.
• Associate the meta-data to the file.
• Stream the file to the database.

A first pass at defining the classes might be as follows:

• Create a FileHandler class responsible for monitoring the folder and moving the file to the secure location
• Create a Document class responsible for creating the meta-data and associating it to the file stream.

For the sake of this blog I’m only going to look at the file handling logic. I do think the FileHandler logic defined above is a defensible breakdown of tasks, the FileHandler object takes care of everything having to do with the files we want to process. What I don’t like about the break down is we have put the file monitoring logic and file transferring logic in the same class. If we change the requirements down the road so that we only look for files with certain extensions we would have to change the FileHandler implementation. If we change the requirements and we need to support a different secure storage location (such as a third party document management system that doesn’t support streaming) we would again have to change the FileHandler. With the high level design I defined above we would have 2 reasons to change the class

1. because the file monitoring logic changes
2. because the file transfer logic changes.

This is a violation of the SRP.

Still not convinced? Let’s look at another change that in my opinion tips the scales in favor of separating the file monitoring and transferring logic. What if we want to allow documents to be imported into the system via a web service? We don’t want to duplicate the file transferring logic so we would want to employ the services of the FileHandler object. We certainly don’t want or need the file monitoring logic in our web service, therefore I favor putting the file monitoring logic and file transferring logic in different classes.

My design review checklist

 

Technorati Tags: SRP,Design Review,Single Responsibility Principle