HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyrailsMinor

Similar methods to update today's sales, revenue, and profit

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
updaterevenuetodaymethodsprofitsalesandsimilar

Problem

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?

Solution

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.

Code Snippets

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
Statistic.create_or_update_today(:sales, network)
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

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

Context

StackExchange Code Review Q#78859, answer score: 4

Revisions (0)

No revisions yet.