patternrubyrailsMinor
Similar methods to update today's sales, revenue, and profit
Viewed 0 times
updaterevenuetodaymethodsprofitsalesandsimilar
Problem
Please help me to refactor. I have two models,
Methods
Statistic model
Conversion model
Is there any way to refactor almost similar pieces of code?
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
endConversion model
class Conversion < ActiveRecord::Base
class << self
def sales_today(network)
where('DATE(created_at)= ? AND networkName = ?', Date.today, network).size
end
etc...
endIs 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:
You would call it like this:
(You'll notice I used
If you wanted you could still define individual methods, but this way it's much cleaner:
You could also do this with
P.S. Just a tip. Don't use
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
This will let the database perform the calculation so ActiveRecord only needs to fetch a single number.
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)
endYou 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).sizeYou 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).countThis 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)
endStatistic.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).sizewhere('DATE(created_at) = ? AND networkName = ?',
Date.today, network).countContext
StackExchange Code Review Q#78859, answer score: 4
Revisions (0)
No revisions yet.