Rails UserHelper method

Question

I need help cleaning up this monstrosity. Basically this is a user helper method that generates an array of cumulative user sign ups per week (start of week == Wednesday) starting 2016-06-15.

So something like this:

[ 1, 5, 9, 22, 34 ]

Where sign ups each week were 1, 4, 4, 13, 12.

In production, this is taking 5 seconds to run (and only 550 users over 7 weeks are present).

  def get_user_array
   @total_date_array = Array.new

    # Remember that the range will go as far as the last Sunday, see next step
    @date_array  = (Date.parse("2016-06-15")..Date.parse(Time.now.strftime("%F"))).select(&:wednesday?)
      @date_array.each do |x|
      start  = x - 7
      finish = x
      @range = start...finish
      @local_user_array = Array.new
      User.all.reject{|u| !u.valid? }.map {|u| @local_user_array << "Yes" if @range === Date.parse(u.created_at.strftime("%F"))}
      @total_date_array <<  @local_user_array
    end

   # Now get the current weeks users
    @current_week_start = @date_array.last
    @current_week_end = Date.parse(Time.now.strftime("%F"))
    @current_week_range = @current_week_start..@current_week_end
    @current_local_user_array = Array.new
    User.all.reject{|u| !u.valid? }.each do |u|
       @current_week_range === Date.parse(u.created_at.strftime("%F")) ? @current_local_user_array << "Yes" : false
    end

    @total_date_array <<  @current_local_user_array

   ## Now create the array....Phew!
   sum = 0
   @total_date_array = @total_date_array.map { |a| Array(a).count("Yes") }.map{ |x| sum  += x }

end

I am fairly inexperienced. I guessing that switching a lot of the code to SQL rather than keeping it in memory is one way to begin.

Answer 1

Ok, so there's a lot to cover here.

• get_user_array is a terrible name. What does "getting a user array" mean? Is it an array of users? A user's array? Be descriptive.

• Why all the instance variables (@...)? This method is a simple helper that has no need to persist state between invocations. Don't use instance variables for this!

• You appear to be laboring under a lot of misunderstandings of, well, fairly basic things to be frank.

  • Don't use #map unless your intention is to transform an array 1-to-1 into a new array. You're using #map a lot, but only once for an actual mapping operation.

  • You're doing a lot of pointless conversion back and forth. For instance, Array(a).count when a is already an array.
    You also do stuff like Date.parse(u.created_at.strftime("%F")) which means you're taking a Date, making it into a String, and then back to a Date again. I'm guessing you may be doing that on purpose to drop the time-part, but doesn't it strike you as an incredibly complicated way of doing that? Why involve strings when going from Date to Date? This sort of thing should cause you to reexamine your approach, because it just doesn't smell right.

  • User.all.reject{|u| !u.valid? } for one, what you want is #select - not #reject. Right now you're saying "do not give me users that are not valid"; with #select, you'd be saying "give me valid users". I.e. User.all.select(&:valid?).
    However, you should really be doing neither! Selecting a subset of records based on given criteria is what databases are for. Your User model should ideally have a scope that'd allow you to just get valid users in one go, e.g. User.valid.all.
    However, you don't really want that either, since you're still pulling every single user for every single week that's passed since mid-June 2016. That's crazy. If you have ~500 user records and it's now been ~7 weeks since your zero-day, you're instantiating 3,500 records only to discard the vast majority with a #reject call! As time passes and user base grows, this gets worse in a hurry. Yes, Rails is smart enough to cache some stuff, but jeez! Doesn't that strike you as a fundamentally bad approach?

Luckily, you can do more stuff in the database. Most, in fact (probably all of it, with more SQL, but I'll leave that for someone better qualified to figure out).

Supposing your User model has a valid scope that restricts queries to only valid records (thus avoid the horrendous #reject stuff), you could do:

START_DATE = "2016-06-15".freeze

# ...

per_week = User
  .valid
  .where("created_at >= ?", START_DATE)
  .order("created_at")
  .group("FLOOR(DATEDIFF(created_at, '#{START_DATE}') / 7)")
  .count

Note: I couldn't get proper query interpolation to work in the group call, so it's just plain string interpolation. Only use this when you 100% control the value to be inserted, otherwise you'll leave yourself open to injection attacks.

This will produce a SQL query that

1. Calculates the number of weeks between the created_at timestamp, and the start date
2. Groups rows sharing the same week
3. Counts the number of rows in each group

The result you get back is something like:

{
  0 => 1,
  1 => 4,
  2 => 4,
  3 => 13,
  4 => 12
}

To then do the cumulative summation you can do:

sum = 0
cumulative_per_week = per_week.map(&:last).map { |_, count| sum += count }

Now, cumulative_per_week will be [1, 5, 9, 22, 34].

I'd also recommend splitting logic into two methods: One for sign-ups per week, and one for cumulative sign-ups per week:

START_DATE = "2016-06-15".freeze

def signups_per_week
  User
    .valid
    .where("created_at >= ?", START_DATE)
    .order("created_at")
    .group("FLOOR(DATEDIFF(created_at, '#{START_DATE}') / 7)")
    .count
    .values
end

def signups_per_week_cumulative
  sum = 0
  signups_per_week.map { |count| sum += count }
end