9

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.

  • 1
  • 6
    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. -- Note that you're conflating "responsibility" with "thing to do." A responsibility is more like an "area of concern." The File class's responsibility is performing file operations. – Robert Harvey Nov 6 '17 at 21:15
  • 1
    It looks to me you're in good shape. All you need is a test mediator (or one for every type of conversion if you like that better). The test mediator may read files to verify their correctness, using .net's File class. There is no issue with that from a SOLID perspective. – Martin Maat Nov 6 '17 at 21:44
  • 1
    As mentioned by @Robert Harvey, SRP has a crappy name because it isn't really about Responsibilities. It is about "encapsulating and abstracting a single tricky/difficult area of concern that might change". I guess STDACMC was too long. :-) That said, I think your division into three parts seems reasonable. – user949300 Nov 6 '17 at 22:04
  • 1
    An important point in your File library from C# is, for all we know the File class could just be a facade, putting all file operations into a single place - into the class, but could be internally using a similar read/write classes to yours which would actually contain the more complicated logic for file handling. Such class (the File) would still adhere the SRP, because the process of actually working with the filesystem would be abstracted behind another layer - most likely with a unifying interface. Not saying it is the case, but it could be. :) – Andy Nov 8 '17 at 9:01
5

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

  • Interesting not seen CQRS before. The code is testable, this is more about trying to find a better way. I use mocks, and dependency injection when I can (which I think is what you are referring to). – James Wood Nov 6 '17 at 22:04
  • First time reading about this, I did identify something similar through my application : handle flexible searches, mutiple fields filterable/sortable, (Java/JPA) is a headache and lead to a tons of boilerplate code, unless you create a basic search engine that will handle this stuff for you (I use rsql-jpa). Though I have the same model (say same JPA Entities for both), the searches are extracted on a dedicated generic service and the model layer don't have to handle it anymore. – Walfrat Nov 15 '17 at 9:33
3

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.

2

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.

  • That's pretty much where I am right now. I can test each function individually, however by testing them they become coupled. E.g. for FileWriter to be tested, then something else has to read what was written, the obvious solution is using FileReader. Fwiw, the mediator often does something else like apply business logic or is perhaps represented by the basic application Main function. – James Wood Nov 6 '17 at 22:09
  • 1
    @JamesWood that's often the case with integration tests. You don't have to couple the classes in test however. You can test FileWriter by reading directly from the filesystem instead of using FileReader. It's really up to you what your goals are in test. If you use FileReader, the test will break if either FileReader or FileWriter is broken--which may take more time to debug. – Samuel Nov 6 '17 at 22:17
  • Also see stackoverflow.com/questions/1087351/… it may help make your tests nicer – Samuel Nov 6 '17 at 22:21
  • That's pretty much where I am right now - that's not 100% true. You said you're using the Mediator pattern. I think this is not useful here; this pattern is used when you have lots of different objects interacting with each other in a very confuse flow; you put a mediator there in order to facilitate all relationships and implement them in one place. This seems to not be your case; you have small units very well defined. Also, like the comment above by @Samuel, you should test one unit, and do your asserts without calling other units – Emerson Cardoso Nov 7 '17 at 15:35
  • @EmersonCardoso; I have somewhat simplified the scenario in my question. Whilst some of my mediators are quite simple, others are more complicated and often use multiple factories/commanders. I'm trying to avoid the detail of a single scenario, I'm more interested in the higher level design architecture that can be applied to multiple scenarios. – James Wood Nov 7 '17 at 17:06
2

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.

  • In that particular example the thing that ties them together is the resource - e.g. the system disk. Otherwise there is no interaction between the two classes. – James Wood Nov 7 '17 at 17:12
1

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.

1

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).

  • "These classes (factory/commander/mediator) shouldn't be aware of file system unless their only job is to store/read provided data." In this particular example, that's all they are doing. – James Wood Nov 8 '17 at 23:17
0

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.

  • 1
    YAGNI is something you should think about. – whatsisname Nov 16 '17 at 4:48

protected by gnat Dec 12 '17 at 15:46

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.