Am I making my classes too granular? How should Single Responsibility Principle be applied?

Question

I write lots of code that involves three basic steps.

1. Get data from somewhere.
2. Transform that data.
3. Put that data somewhere.

I typically end up using three types of classes - inspired by their respective design patterns.

1. Factories - to build an object from some resource.
2. Mediators - to use the factory, perform the transformation, then use the commander.
3. Commanders - to put that data somewhere else.

My classes tend to me be quite small, often a single (public) method, e.g. get data, transform data, do work, save data. This leads to a proliferation of classes, but in general works well.

Where I struggle is when I come to testing, I end up will tightly coupled tests. For example;

• Factory - reads files from disk.
• Commander - writes files to disk.

I can't test one without the other. I could write additional 'test' code to do disk read/write also, but then I'm repeating myself.

Looking at .Net, the File class takes a different approach, it combines the responsibilities (of my) factory and commander together. It has functions for Create, Delete, Exists, and Read all in one place.

Should I look to to follow the example of .Net and combine - particularly when dealing with external resources - my classes together? The code it still coupled, but it's more intentional - it happens at the original implementation, rather than in the tests.

Is my issue here that I have applied Single Responsibility Principle somewhat overzealously? I have separate classes responsible for read and write. When I could have a combined class which is responsible for dealing with a particular resource, e.g. system disk.

Answer 1

Following the Single Responsibility principle may have been what guided you here but where you are has a different name.

Command Query Responsibility Segregation

Go study that and I think you'll find it following a familiar pattern and that you're not alone in wondering how far to take this. The acid test is if following this is getting you real benefits or if it's just a blind mantra you follow so you don't have to think.

You've expressed concern about testing. I don't think following CQRS precludes writing testable code. You might simply be following CQRS in a way that makes your code not be testable.

It helps to know how to use polymorphism to invert source code dependencies without needing to change the flow of control. I'm not really sure where your skill set is on writing tests.

A word of caution, following the habits you find in libraries is not optimal. Libraries have their own needs and are frankly old. So even the best example is only the best example from back then.

This isn't to say there aren't perfectly valid examples that don't follow CQRS. Following it will always be a bit of a pain. It's not always one worth paying. But if you need it you'll be glad you used it.

If you do use it heed this word of warning:

In particular CQRS should only be used on specific portions of a system (a BoundedContext in DDD lingo) and not the system as a whole. In this way of thinking, each Bounded Context needs its own decisions on how it should be modeled.

Martin Flowler: CQRS

Answer 2

You need a broader perspective to determine if code conforms to The Single Responsibility Principle. It cannot be answered just by analyzing the code itself, you have to consider what forces or actors might cause the requirements to change in the future.

Lets say you store application data in an XML file. What factors could cause you to change the code related to reading or writing? Some possibilities:

• The application data model could change when new features are added to the application.
• New kinds of data - e.g. images - could be added to the model
• The storage format could be change independent of application logic: Say from XML to JSON or to a binary format, due to interoperability or performance concerns.

In all these cases, the you will have to change both the reading and the writing logic. In other words, they are not separate responsibilities.

But lets imagine a different scenario: You application is part of a data processing pipeline. It reads some CSV files generated by a separate system, performs some analysis and processing and then output a different file to be processed by a third system. In this case the reading and writing are independent responsibilities and should be decoupled.

Bottom line: You cannot in general say if reading and writing files are separate responsibilities, it depends on the roles in the application. But based on your hint about testing, I would guess it is a single responsibility in your case.

Answer 3

Generally you have the right idea.

Get data from somewhere. Transform that data. Put that data somewhere.

Sounds like you have three responsibilities. IMO the "Mediator" may be doing to much. I think you should start by modeling your three responsibilities:

interface Reader[T] {
    def read(): T
}

interface Transformer[T, U] {
    def transform(t: T): U
}

interface Writer[T] {
    def write(t: T): void
}

Then a program can be expressed as:

def program[T, U](reader: Reader[T], 
                  transformer: Transformer[T, U], 
                  writer: Writer[U]): void =
    writer.write(transformer.transform(reader.read()))

This leads to a proliferation of classes

I don't think this is a problem. IMO lots of small cohesive, testable classes are better than large, less-cohesive classes.

Where I struggle is when I come to testing, I end up will tightly coupled tests. I can't test one without the other.

Each piece should be independently testable. Modeled above, you can represent reading/writing to a file as:

class FileReader(fileName: String) implements Reader[String] {
    override read(): String = // read file into string
}

class FileWriter(fileName: String) implements Writer[String] {
    override write(str: String) = // write str to file
}

You can write integration tests to test these classes to verify they read and write to the filesystem. The rest of the logic can be written as transformations. For example if the files are JSON format, you can transform the Strings.

class JsonParser implements Transformer[String, Json] {
    override transform(str: String): Json = // parse as json
}

Then you can transform to proper objects:

class FooParser implements Transformer[Json, Foo] {
    override transform(json: Json): Foo = // ...
}

Each of these is independently testable. You can also unit test program above by mocking reader, transformer, and writer.

Answer 4

I end up will tightly coupled tests. For example;

• Factory - reads files from disk.
• Commander - writes files to disk.

So the focus here is on what is coupling them together. Do you pass an object between the two (such as a File?) Then it's the File they're coupled with, not each other.

From what you have said you have separated your classes. The trap is that you're testing them together because it's easier or 'makes sense'.

Why do you need the input to Commander to come from a disk? All it cares about is writing using a certain input, then you can verify it wrote the file correctly using what is in the test.

The actual part you are testing for Factory is 'will it read this file correctly and output the right thing'? So mock the file before reading it in the test.

Alternatively, testing that the Factory and Commander work when coupled together is fine - it falls in line with Integration Testing quite happily. The question here is more a matter of whether or not your can Unit Test them separately.

Answer 5

Get data from somewhere. Transform that data. Put that data somewhere.

It's a typical procedural approach, one that David Parnas wrote about back in 1972. You concentrate on how things are goings. You take the concrete solution of your problem as a higher-level pattern, which is always wrong.

If you pursue object-oriented approach, I'd rather concentrate on your domain. What is it all about? What are your system's primary responsibilities? What are the main concepts that present in the language of your domain experts? So, understand your domain, decompose it, treat higher-level areas of responsibility as your modules, treat lower-level concepts represented as nouns as your objects. Here is an example I provided to a recent question, it is very relevant.

And there is an evident problem with cohesiveness, you've mentioned it yourself. If you make some modification is an input logic and write tests on it, it's in no way proves that your functionality works, since you could forget to pass that data to the next layer. See, these layers are intrinsically coupled. And an artificial decoupling makes things even worse. I know that myself: 7 y.o. project with 100 man-years behind my shoulders written completely in this style. Run away from it if you can.

And on the whole SRP thing. It's all about cohesion applied to your problem space, i.e., domain. That's the fundamental principle behind SRP. This results in objects being smart and implementing their responsibilities for themselves. No one controls them, no one provides them with data. They combine data and behavior, exposing only the latter. So your objects combine both raw data validation, data transformation (i.e., behavior) and persistence. It could look like the following:

class FinanceTransaction
{
    private $id;
    private $storage;

    public function __construct(UUID $id, DataStorage $storage)
    {
        $this->id = $id;
        $this->storage = $storage;
    }

    public function perform(
        Order $order,
        Customer $customer,
        Merchant $merchant
    )
    {
        if ($order->isExpired()) {
            throw new Exception('Order expired');
        }

        if ($customer->canNotPurchase($order)) {
            throw new Exception('It is not legal to purchase this kind of stuff by this customer');
        }

        $this->storage->save($this->id, $order, $customer, $merchant);
    }
}

(new FinanceTransaction())
    ->perform(
        new Order(
            new Product(
                $_POST['product_id']
            ),
            new Card(
                new CardNumber(
                    $_POST['card_number'],
                    $_POST['cvv'],
                    $_POST['expires_at']
                )
            )
        ),
        new Customer(
            new Name(
                $_POST['customer_name']
            ),
            new Age(
                $_POST['age']
            )
        ),
        new Merchant(
            new MerchantId($_POST['merchant_id'])
        )
    )
;

As a result there are quite a few of cohesive classes representing some functionality. Note that validation typically goes to value-objects -- at least in DDD approach.

Answer 6

Where I struggle is when I come to testing, I end up will tightly coupled tests. For example;

• Factory - reads files from disk.
• Commander - writes files to disk.

Watch out for leaky abstractions when working with file system - I saw it neglected way too often, and it has the symptoms you've described.

If the class operates on data that comes from/goes into these files then file system becomes implementation detail (I/O) and should be separated from it. These classes (factory/commander/mediator) shouldn't be aware of file system unless their only job is to store/read provided data. Classes that deal with file system should encapsulate context specific parameters like paths (might be passed through constructor), so the interface didn't reveal its nature (word "File" in interface name is a smell most of a time).

Answer 7

In my opinion it sounds like you've started heading down the right path but you haven't taken it far enough. I think breaking up the functionality into different classes that do one thing and do it well is correct.

To take it a step further you should create interfaces for your Factory, Mediator, and Commander classes. Then you can use mocked out versions of those classes when writing your unit tests for the concrete implementations of the others. With the mocks you can validate that methods are called in the correct order and with the correct parameters and that the code under test behaves properly with different return values.

You could also look at abstracting the reading/writing of the data. You're going to a file system now but may want to go to a database or even a socket sometime in the future. Your mediator class shouldn't have to change if the source/destination of the data changes.