4
\$\begingroup\$

Please help me to refactor. I have two models, conversion & statistic. Methods sales_today, revenue_today, profit_today in conversion model only gets specific info from db.

Statistic model

class Statistic < ActiveRecord::Base

class << self 

def create_and_update_today(agregate_name, value, network)
  today = find_or_create_by(agregateName: agregate_name, networkName: network)
  today.update(agregateName: agregate_name, date: Date.today, 
                value: value, networkName: network)
end

def sales_today(network)
  total = Conversion.sales_today(network)
  create_and_update_today('salesToday', total, network)
end

def revenue_today(network)
  total = Conversion.revenue_today(network)
  create_and_update_today('revenueToday', total, network)
end

def profit_today(network)
  total = Conversion.profit_today(network)
  create_and_update_today('profitToday', total, network)
end

end
end

Conversion model

class Conversion < ActiveRecord::Base

  class << self

def sales_today(network)
  where('DATE(created_at)= ?  AND networkName = ?', Date.today, network).size
end
etc...

end

Is there any way to refactor almost similar pieces of code?

\$\endgroup\$
  • 2
    \$\begingroup\$ I'm not sure I understand: which class is this code excerpt from? Could you post the Conversion class as well? \$\endgroup\$ – 200_success Jan 28 '15 at 11:25
4
\$\begingroup\$

Instead of dynamically defining a bunch of methods, you could just have one method that takes an argument that tells it what kind of work to do:

def self.create_or_update_today(kind, network)
  meth = :"#{kind}_today"
  total = Conversion.send(meth, network)

  aggregate_name = "#{kind}Today"

  today = find_or_initialize_by(agregateName: aggregate_name, networkName: network)    
  today.update(date: Date.today, value: total)
end

You would call it like this:

Statistic.create_or_update_today(:sales, network)

(You'll notice I used find_or_initialize_by instead of find_or_create_by. This way the code will always perform two queries instead of performing three if it's a new record. update will create the record of it doesn't exist.)

If you wanted you could still define individual methods, but this way it's much cleaner:

def self.sales_today(network)
  self.create_or_update_today(:sales, network)
end

def self.revenue_today(network)
  self.create_or_update_today(:revenue, network)
end

# ...

You could also do this with define_method in a loop as Devon does, but it would save very few lines while sacrificing a lot of readability and, as britishtea points out, introducing documentation issues.

P.S. Just a tip. Don't use size for this:

where('DATE(created_at) = ?  AND networkName = ?',
      Date.today, network).size

You only want to know how many records there are, but when you do this it fetches all of the data in all of the records and initializes a Conversion object for each, which is really wasteful. Instead of size, use Rails' built-in count method:

where('DATE(created_at) = ?  AND networkName = ?',
      Date.today, network).count

This will let the database perform the calculation so ActiveRecord only needs to fetch a single number.

\$\endgroup\$
3
\$\begingroup\$

In addition to Devon's answer. You can put method definitions right in the body of class:

class Statistic < ActiveRecord::Base

  class << self 
    [:sales_today, :revenue_today, :profit_today].each do |method_name|
      define_method method_name do |network|
        total = Conversion.send(method_name, network)
        create_and_update_today(method_name.to_s.camelcase(:lower), total, network)
      end
    end

    def create_and_update_today(agregate_name, value, network)
      today = find_or_create_by(agregateName: agregate_name, networkName: network)
      today.update(date: Date.today, value: value)
    end
  end
end

Also I've removed aggregate and network names from #update call, as they won't update anyway.

\$\endgroup\$
2
\$\begingroup\$

You can do some nifty meta-programming to dynamically define the methods.

The code below is slightly modified to show the results, you should find it trivial to change back to suit your needs.

class Foo < ActiveRecord::Base
  def initialize
    super
    [:sales_today,:revenue_today,:profit_today].each do |m|
       self.class.send(:define_method, m) do |network|
        total = 2 # Conversion.sales_today(network)
        # create_and_update_today(m, total, network)
        do_work(m, total, network)
      end
    end
  end

  def do_work (one, two, three)
    puts "#{one} #{two} #{three}"
  end
end

foo = Foo.new

puts "Methods: #{foo.methods}"
foo.sales_today("network 1")
foo.revenue_today("network 2")
foo.profit_today("network 3")

Output:

Methods: do_work
sales_today
revenue_today
profit_today
nil?
===

...

sales_today 2 network 1
revenue_today 2 network 2
profit_today 2 network 3
\$\endgroup\$
  • \$\begingroup\$ Won't this redefine those class methods every time a new instance of Foo is initialized? \$\endgroup\$ – Jordan Jan 29 '15 at 6:52
  • \$\begingroup\$ @Jordan Hmm, I think it will, let me look into this further to see if I can prevent that \$\endgroup\$ – Devon Parsons Jan 29 '15 at 9:34
  • \$\begingroup\$ Keep in mind that defining methods dynamically like this, keeps them out of your documentation. You might prefer a little duplication over methods hidden in documentation. \$\endgroup\$ – britishtea Jan 29 '15 at 10:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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