Keeping Domain Models Cohesive with Collaborators

Posted on by in Process, Web

As an application matures, classes begin to take on more and more responsibilities. Eventually a class’s main responsibility starts to become obscured. You can prevent overwhelming your classes by introducing collaborators to help them fulfill their responsibilities. In this post, we’ll look at an example of using a collaborator to prevent non-domain responsibilities from creeping into a domain model.

Bank Accounts and CSV Parsers

Our sample app is a banking web app. Currently it has a single domain model: Account. Account represents both checking and savings accounts. Each account has a name, an account type (checking or savings), and a balance.

Our users want to be able to import accounts from a CSV file. Let’s start with a failing integration test.

spec/requests/importing_accounts_spec.rb

And here’s the above CSV file of accounts.

spec/fixtures/accounts.csv

Running this gives us our first failing test.

For simplicity, we’ll skip directly unit testing our view and controller, and instead move straight to the implementation.

First, we define some routes.

config/routes.rb

Then, a homepage view with a file upload form.

app/views/accounts/index.html.haml

And finally, a simple controller to handle the file upload request.

app/controllers/accounts_controller.rb

After this basic implementation, our test is still failing.

Great. We can now move down to the domain model.

Isolating Non-Domain Responsibilities

Before we jump into the Account unit test, let’s consider two possible CSV importing implementations:

  • Account parses the CSV file and imports the accounts
  • Account asks another object to parse the CSV file and import the accounts

In our domain, the Account model represents bank accounts. CSV parsing has nothing to do with banking. If we changed from CSV to plaintext, it would be strange to have to change Account. Also, if you were investigating a bug with CSV account importing, where would you look? The Account model is probably your first choice, but CSV importing is most likely going to be hard to find amongst Account‘s other responsibilities. Having a separate object dedicated to CSV account importing makes it more obvious to other developers as to where this behavior exists. Let’s take this latter approach.

spec/models/account_spec.rb

In this test, we introduce and stub out an account importer object. An account importer will take a file and return the total number of accounts imported from the file.

Let’s get this test passing by having Account tell the account importer to import the file.

app/models/account.rb

Our unit test is passing, but our integration test is still failing.

It’s failing because we’re not passing an account importer to Account::import.

app/controllers/account_controller.rb

Let’s go back down to the domain model and test drive this CsvAccountImporter.

spec/models/csv_account_importer_spec.rb

And here’s a possible implementation in order to get this test passing.

app/models/csv_account_importer.rb

Our integration test should now be passing.

Hiding Implementation Details with Default Arguments

Since our tests are now green, it’s time to refactor. In our controller, we explicitly passed the Account‘s collaborator, a CsvAccountImporter. But a controller shouldn’t know how Account imports CSV accounts. We can prevent this implementation detail from leaking out of our domain model by using Ruby’s default arguments.

First, we’ll delete the CSV account importer from the controller.

app/controllers/accounts_controller.rb

Then, we’ll set a new CsvAccountImporter object as the default account importer argument.

app/models/account.rb

Now our controller knows less and Account‘s collaborator is much more obvious.

Leaner Models

Every codebase usually has that one massive class. It weighs in at over 1000 lines; its tests are twice as long and have no organization whatsoever. Since it contains so much behavior, it’s changed on almost every commit. Developers fear changing it because of merge conflicts caused from other developers also changing it. Collaborators are a simple way to break apart these unmaintainable classes. Don’t be afraid to add another class. Remember that your motivation is clarity, not flexibility.


Feedback

  Comments: 16


  1. CSVAccountImporter has a hardcoded reference to Account and its attributes. Could that be too tight of a coupling?


    • In my opinion that coupling is fine. CsvAccountImporter isn’t meant to be super generic. It’s meant to translate from the non-domain world to the domain, so I think it’s fine for it to know about Account.


  2. Nice post Jared. I like your approach to hiding the implementation details of the importer from the controller using default arguments. The default arguments approach also looks like a nice little way to inject the importer. The method signature exposes an interface for other possible implementations of an importer (i.e. different formats). Was that a side effect of your approach?

  3. Michael Wynholds


    I don’t really like Account being the entry point for the CSV import, and I don’t like Account knowing about CsvAccountImporter. You are not really separating the domain from the application-specific logic. You’re just moving around the code. So the code is cleaner, but the architecture is not.

    I would propose having the CsvAccountImporter be the entry point, and it should know about accounts. Perhaps it creates a bunch of Account objects, or even a bunch of generic Hashes, and passes that to an Account.import method. You may very well have some domain-specific logic around mass importing accounts (how to handle duplicates, for example), but CSV parsing is not part of that.

    I think application logic should always sit on top of domain logic, and nothing in the domain should know about anything in the application level. Just like you’d never have the Account object make a call to the AccountController, you should also never have it make a call to the AccountImporter (or CsvAccountImporter).

    In your example, the AccountController, itself being in the application level, can know about the CsvAccountImporter, because it is also in the application level. So the call chain goes AccountController –> CsvAccountImporter –> Account.

    I’d love to hear your thoughts on this, Jared.


    • I think I prefer that sort of structure as well. Allowing Accounts to be responsible for importing themselves (even if they delegate to a collaborator) seems like giving the model too many roles. In the architecture above CsvAccountImporter becomes a service layer class which seems to me like a clearer way to manage these responsibilities.

      That said collaborators should be a useful strategy pattern for cases where we may want to have well contained, interchangeable, domain logic in out models.


      • Do you think that introducing a service layer would rob the domain model of behavior (even if a domain object is delegating entirely to another object)?


    • If the CsvAccountImporter was to be the entry point for creating an account(s) then I would rethink the action used. Instead of AccountController#create i’d probably move the importer logic to AccountController#import. I’ve run into this issue before, and if sticking with the restful nature of the controller, opted to either create a new controller that wasn’t pinned specifically to the model or to another action.


      • Are you assuming that CsvAccountImporter persists the Accounts? I think I would want the importer to instantiate the models but return them to the controller’s create action to handle persistence.


        • I actually see the value of having the class-level (or in other cases instance level) do it. BUT I don’t like the idea of explicitly defaulting the importer. Or at least make it a JSON importer ;-)


    • I think introducing an application/service layer would work great in further decoupling the domain model from app-specific logic (CSV parsing). My implementation keeps app-specific logic in the domain but in separate objects. An application/service layer would be a logical next step.

  4. Víctor Martínez


    To me this design is completely broken. AccountImporter should take a file and use its mime type to process it or raise and exception if not supported. It is not the model nor the controller responsibility to know how the file should be processed but the importer itself.

    • Víctor Martínez


      In fact this problem probably fits well into the adapter pattern.


    • I think making a more generic AccountImporter that’s capable of parsing multiple types of files is a perfectly, reasonable change. And having the controller talk directly to this AccountImporter would allow you to remove it from the Account model.

      Once you start supporting multiple types of files, you could probably refactor the AccountImporter into subclasses to handle the differences in file parsing.


    • That’s sounds like a perfectly, reasonable design. You could then make the controller talk directly to the AccountImporter. That would remove account importing from the domain.

      Once you start supporting multiple file types, you could refactor the AccountImporter into subclasses to account for differences in file parsing.

Your feedback