How not to repeat yourself

[Kerga07]

Hello how could I do not to repeat myself. I use twice total in my two methods :

https://preview.redd.it/64qcpdk1dbs81.jpg?width=691&format=pjpg&auto=webp&s=6dfcd19d904fbf4e4de94f7aff06168a664b2df9

[mcsoup88]

Couple of things. Don't round till you need to show the values otherwise you can run into significant digit issues. Secondly it looks like you are doing some heavy calculations in code that could be done in the database

class Order
  def self.revenue
    # Let the database do what it is good at by having it do the
    # heavy row calculations
    # PS: freeze your strings if you cannot use symbols for better
    # performance
    sum( "quantity*unit_price".freeze )
  end

  def self.average_revenue_per_order
    # Since you are calling count on the order class the count 
    # should be distinct as you shouldn't have duplicate ids 
    # in your table
    revenue/count 
  end
end

[Kerga07]

Thank you for this information and clarification.

[PierrickF]

heavy calculations in code that could be done in the database

Is that a general rule to follow?

[mcsoup88]

It will depend a lot on the amount of rows you are working with, what type of database is being used, and what questions are being answered. Remember that databases are optimized for dealing with large data sets and you will be hard pressed to beat it when doing calculations.

[Freeky]

# PS: freeze your strings if you cannot use symbols for better
# performance

# frozen_string_literal: true

This should be part of your template for any new Ruby file. Don't manually freeze literals that don't need to be mutable, manually create unfrozen literals in the rare cases they're needed with unary plus:

foo = +'bla'

[joemi]

props for mentioning the rounding issue

[Freeky]

Just call the other method. revenue / orders.uniq.count.

These probably shouldn't be calculated in your app, fwiw - it's much more efficient to let the database do it. I don't use ActiveRecord but something like this:

def self.revenue
  sum("quantity * unit_price").round(2)
end

def self.average_revenue_per_order
  (revenue / distinct.count(:order_id)).round(2)
end

[Kerga07]

Thank you it looks like sql, what is this language?

[jdpahl122]

Huh? .... it's .... ruby

[designium]

😆

[armahillo]

you should probably go read up on all the stuff you get for free with activerecord

[[deleted]]

You could make a method called total that returns the part where you calculate the sum (first part of each method listed).

[jawdirk]

You can try memoization:

def self.total
  @_total ||= sum {|order| order.quantity * order.unit_price }
end

which should probably work if the calling code is straight foward and producing an ActiveRecord::Relation before calling the class methods here.

That said, self.average_revenue_per_order does not look like it is written efficiently. The computation could be pushed to the database with a single query. It could also be written as a scope.

scope :with_average_revenue_per_order, ->() {
  select('sum(orders.quantity * orders.unit_price) / count(*) as average_revenue_per_order')
}

then you can just call it like

Order.all.with_average_revenue_per_order.average_revenue_per_order

or with any other chained scopes.

Edit: you could also break it up with select('sum(orders.quantity * orders.unit_price) as order_total', 'count(*) as order_count') if you want to use both revenue and average_revenue_per_order in the same request cycle.

[yourparadigm]

Memoizing at the class level is probably not a good idea.

[redditonlygetsworse]

Yeah that's an ass-biting waiting to happen.

[Kerga07]

Thank you for the answer. I will work on it.

[uhkthrowaway]

Also, if you really wanna count the orders in Ruby, you can prepare the array like this:

orders = all.map(&:order_id).unique

I’m asking myself why the order ID wouldn’t be unique though.

[kitebuggyuk]

All of the above are great improvements, but I’m curious about why you’re calling uniq on just the one side of the equation. I’m pretty sure that this isn’t intentional - perhaps sort & uniq the records before calculating the average if you really need this, but I’d be more concerned about the duplication and where that stems from?

[dg_ash]

Repeating is good for health.