← Back to Upcase

Too much indirection? Really? I don't get it


(Rafael George) #1

Hi guys,

I was building this CRUD feature for an app and find myself trying to with an outside in approach for the code; and ended with not very Rails Way ‘way’ of doing things as my peers at work told me, saying that what do you think are really the problems of this particular code? and also of the tests. As you can see I’m trying to isolate myself from Rails entirely; also following Sandi Metz Magic Tricks on Testing rules.

module Admin 
  class DomainsController < Admin::ApplicationController
    def index 
      @unknown_domains = DomainConfigs::ListUnknowns.list(DomainConfig)
      @attempted_signups = AttemptedSignups::ListRecent.list(AttemptedSignup)
    end

    def new 
      @domain = DomainConfig.new
      @attempted_signups = []
      @imap_configurations = []
    end

    def edit 
      @attempted_signups = AttemptedSignups::LastFailed.list(
        AttemptedSignup, domain_config.domain)
      @imap_configurations = ImapConfigurations::Last.list(
        ImapConfiguration, domain_config.domain)
    end

    def create
      @domain = DomainConfigs::Creator.create(DomainConfig, params[:domain_config])
      @attempted_signups = []
      @imap_configurations = []
      if @domain.valid?
        flash[:notice] = 'Domain config created successfully' 
        redirect_to admin_domain_configs_path
      else 
        flash[:error] = 'Domain config did not get created' 
        render action:'new'
      end
    end
end
module DomainConfigs

  class Creator 
    def self.create(domain_config_repo, attributes)
      new(domain_config_repo).create(attributes)
    end

    def initialize(domain_config_repo)
      @domain_config_repo = domain_config_repo
    end

    def create(attributes)
      domain_config_repo.save_domain_config(attributes.fetch(:status),
                                            attributes.fetch(:host), 
                                            attributes.fetch(:domain),
                                            attributes.fetch(:username_hint),
                                            attributes.fetch(:username_format),
                                            attributes.fetch(:account_type),
                                            attributes.fetch(:probe_methods))
    end

    private
    attr_reader :domain_config_repo
  end

end
require 'minitest/autorun'
require_relative '../../../lib/domain_configs/creator'

module DomainConfigs
  class CreatorTest < Minitest::Unit::TestCase
    def test_create
      domain_config = Minitest::Mock.new
      attributes = {status:'status',
                    host:'host',
                    domain:'domain',
                    username_hint:'username_hint',
                    username_format:'username_format',
                    account_type:'account_type',
                    probe_methods:'probe_methods'}
      domain_config.expect(:save_domain_config, true, ['status', 
                                                       'host', 
                                                       'domain', 
                                                       'username_hint', 
                                                       'username_format',
                                                       'account_type',
                                                       'probe_methods']) 
      assert_equal true, Creator.create(domain_config, attributes)
      domain_config.verify
    end

  end
end
module DomainConfigs 
  class ListUnknowns
    def self.list(domain_config)
      domain_config.unknown_domain_configs
    end
  end
end
require_relative '../../../lib/domain_configs/list_unknowns'

module DomainConfigs
  class ListUnknownsTest < Minitest::Unit::TestCase
    class DomainConfig < Struct.new(:host, :status, :domain, :account_type)
    end

    def test_list 
      domain_config = Minitest::Mock.new
      domain_config.expect :unknown_domain_configs, []

      DomainConfigs::ListUnknowns.list(domain_config)
      domain_config.verify
    end
  end
end

I just paste some of the services and code; because the other services follow the same approach. The models have wrapped methods which encapsulate what I’m calling from the outside. My peers told me that I have too much inderiction on this and that I have needless abstraction that I probably should just call create!```` and ````update_attributes! from within the controller and test actual logic not just expecting the call to be made from my service layer.

This is actually confusing for me because Sandi says that if we have an external dependency which do something in another place then we should just expect that method to be call and not test for side effects from the caller if that’s.

Any suggestion will be appreciate it; thanks :smile:


(Derek Prior) #2

I think I agree with your colleagues here, at least in the parts of the code I’ve skimmed through. For instance, DomainConfigs::Creator appears to be an object that exists solely to wrap DomainConfig.save_domain_config. Do I have that right?

In that entire create flow, #save_domain_config is actually what piques my interest most. What’s going on there that is different than just create? Is saving a domain configuration a multi-step operation or something that has callbacks? If so, that is the object I’d extract here. It sounds like it’s simply a method that is meant to isolate from the rails api of create. If that’s the case, I’d say just use create. If you moved away from rails, you’d need to implement create.

In general I insert objects between controllers and models in cases where the existing models don’t support the functionality I’m after (and shouldn’t). In other cases, I’d prefer to just keep it simple and vanilla until I have a specific reason for another abstraction.

This is actually confusing for me because Sandi says that if we have an external dependency which do something in another place then we should just expect that method to be call and not test for side effects from the caller if that’s.

And I think this makes perfect sense, but you have to balance that with the realities of dealing with ActiveRecord objects. By all means, push business logic out of your persistence (AR) layer if you want, but what you seem to be doig here is wrapping the persistence in something else. I don’t think there’s much win there.


(Rafael George) #3

@derekprior Thanks for your reply; you are correct the #save_domain_config just wrap the create method from AR. I was trying to make wrap methods of the API of AR and not use mocks with my objects on things that I don’t control like AR external API. Following suggestions from Gary Benhardt on this probably this is a needless abstraction as my peer say. What do you think about the other objects on this iteration I’m in the process of refactoring this entirely and just with the Rails way which to be honest I really don’t like for most of the time for starters I always have to wait like 2 minutes to wrong a single test file with just 5 assertions for the different actions on this controller which is slow in my book; I can think of this tests as integration tests which they are and just write those and forget about isolating and moving business logic out of Rails. Don’t know if this will be a good approach overall regarding the healthyness of this app. Anyways thanks for your feedback :slight_smile:


(Luís Ferreira) #4

If you want to go the distance on what Gary Bernhardt talks about and to some extent Sandi Metz as well, you might want to check out this post (disclosure: I wrote it), and be on the watch for this framework.

Both of them rely mostly on ruby, not rails. Even though you can still use AR.


(Rafael George) #5

@zamith Thanks, I’ve been taking a look into Lotus already a lot of good stuffs coming. I will read your blog post and come back to talk about it :wink: