Here is a contrived example of a common SOLID violation you might see. Can you spot it?
class Mp3Encoder : IMp3Encoder
{
public void Encode(IEnumerable<string> wavFiles)
{
foreach (var wavFile in wavFiles)
{
var outputFile = /* create output file */;
while (/* blocks remaining... */)
{
var buffer = /* read block */;
var encoded = /* encode wav block as MP3 */;
/* write block to output file */;
}
/* write ID3 trailing header */;
}
}
}
Except in trivially simple cases, there should always be a class boundary when shifting context from coordinating a collection versus performing actions on a single object.
The class above is violating this rule — it knows how to perform collection-level responsibilities as well as single-object responsibilities. It needs to be broken into two classes; one for encoding a single file and one for coordinating the group.
This rule is a form of the Single Responsibility Principle. For example:
Collection-scoped class responsibilities
- Coordinating ‘before all’ and ‘after all’ actions
- Looping through items
- Maintaining shared state (counting, accumulating etc)
Single object-scoped class responsibilities
- Coordinating ‘before each’ and ‘after each’ actions
- Performing actions on item
If you ignore this collection-vs-single-object contextual boundary, your classes will become messes of nested procedural code — especially when different behaviour is required for each item in the collection. Your classes will be that much harder to unit test, and you won’t easily be able to re-use them in single-object scenarios.
December 30, 2011
2 Comments
David on December 30, 2011 at 10:17 pm.
I would also say that the Encoder creating the file is also a violation of SRP. The Encoder should simply result in a stream, which then something else would write out to a file, or an HTTP response, or a memory location, etc.
Richard on December 31, 2011 at 9:06 am.
@David: definitely. Same for Id3TagWriter etc.