CODEWITHSUNDEEP
ruby-on-railsruby
mdkrog
mdkrog

Reputation: 378

Correct implementation of dependency injection

I am torn between the best way to implement this service:

Currently I have this:

class ReissueInvoices

  def initialize(registration, invoice_gateway, invoice_generator)
    @registration = registration
    @invoice_gateway = invoice_gateway
    @invoice_generator = invoice_generator
  end

  def call
    void_current_invoices
    regenerate_invoices
    create_third_party_invoices
  end

  private

  attr_reader :registration, :invoice_gateway, :invoice_generator

  def void_current_invoices
    registration.invoices.each do |invoice|
      unless invoice.paid?
        invoice_gateway.void_invoice(invoice)
      end
    end
  end

  def regenerate_invoices
    invoice_generator.call(registration)
  end

  def create_third_party_invoices
    invoice_gateway.create_invoices(registration)
  end

end

and I call this (normally from my controller) like this:

ReissueInvoices.new(@registration, InvoiceGateway.new, InvoiceGenerator.new).call

I obviously have an InvoiceGateway, InvoiceRegistration class and pass these in as a dependency to my ReissueInvoices class.

Is this the best way to do things? Is this correctly implementing dependency injection? or should I change my ReissueInvoices class to something like this removing the parameters from the initialize method and adding private methods to create and access the invoice_generator and invoice_gateway objects:

class ReissueInvoices

  def initialize(registration)
    @registration = registration
  end

  def call
    void_current_invoices
    regenerate_invoices
    create_third_party_invoices
  end

  private

  attr_reader :registration

  def invoice_gateway
    @invoice_gateway ||= InvoiceGateway.new
  end

  def invoice_generator
    @invoice_generator ||= InvoiceGenerator.new
  end

.....

and call it like so

ReissueInvoices.new(@registration).call

Lastly, what do you guys think of defining default parameters like this in the initializer:

def initialize(registration, invoice_gateway=InvoiceGateway.new, invoice_generator=InvoiceGenerator.new)
   @registration = registration
   @invoice_gateway = invoice_gateway
   @invoice_generator = invoice_generator
 end

Good or bad?

Thanks,

Matt

Upvotes: 0

Views: 71

Answers (1)

Joshua Cheek
Joshua Cheek

Reputation: 31726

Why does your gateway not need something like credentials? I'm guessing it does, and the gateway's credentials are just hard-coded. Really, that should be initialized somewhere and then passed in (because your ReissueInvoices class shouldn't have to deal with how to setup a gateway, presumably there is only one gateway and you should use that same object everywhere).

I wouldn't bother passing in the invoice generator unless it's expensive or you need control over what it returns for a test or there's multiple ways to generate the thing, and you can't reasonably know which is right from here. This is just because it's a thing that does some calculation and returns a value. Nice and simple, barring considerations like the ones I mentioned.

Also, I'd probably change it to be something like this:

module ReissueInvoices
  def self.call(registration, invoice_gateway)
    registration.invoices.each do |invoice|
      invoice_gateway.void_invoice(invoice) unless invoice.paid?
    end
    InvoiceGenerator.call(registration)
    invoice_gateway.create_invoices(registration)
  end
end

The thought process went approximately like this:

  • I'm never going to look at that call method and not need to look at the methods they implement, so lets see what they do. Oh, they're just hiding the collaborators.
  • Pull them up into #call
  • "Why am I instantiating an InvoiceGenerator? It takes no arguments, so it shouldn't have state, so no need to instantiate. I assume it only has the one method, since the method is named #call, so just make it a singleton method and if there's some value to instantiating, the singleton method can make this decision.
  • Now that everything is in ReissueInvoices#call, what is left? It's just wiring to support the objectness of this action (#initialize and accessors). Instead, make it a class method named call. Then, these become local vars.
  • Now that we only have the one class method, making this a class is misleading, since it isn't expected to be instantiated. So make it a module instead.

Upvotes: 1

Related Questions