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
- Calculates the number of weeks between the
created_at
timestamp, and the start date
- Groups rows sharing the same week
- 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