I am implementing a feature that reminds users to make a post via email if

  • the user has set daily reminders to true
  • he has not posted yet today
  • and if the current hour matches when he would like to receive the daily reminder in his time zone.

I am going to write a Rake task for it and then schedule it to run every hour.

Currently, the rake tasks looks like this:

namespace :mail do
  desc "Send daily reminder to users"
  task :daily_reminder => :environment do
    User.all.each do |user|
      Time.zone = user.time_zone
      if user.reminder == true && Time.current.strftime("%H").to_i == user.reminder_time && user.reminded == false
        user.update_attribute(:reminded, true)

      elsif Time.current.strftime("%H") == user.reminder_time + 1 && user.reminded == true
        user.update_attribute(:reminded, false)
        puts "Update to false"

I am resetting the field reminded back to false the hour after they were reminded.

Since I am rather new to Ruby and Ruby on Rails I would appreciate feedback on the setup of this feature a lot.

Please critique away on anything you think is broken or needs improvement.

Also, right now it is not working and it's giving me back a undefined method + for nil:NilClass which tells me that the user hasn't an ActiveRecord assigned to it? Because the reminder_time column exists and is set (to 16:00).

Follow-up question:

  • The if clause looks kind of large and unclean. Is that the proper way to go about this?
  • Shall I make the whole logic more clean by putting some of the conditions in methods?

1 Answer 1

  1. Never User.all.each in the real world. That loads your entire table into memory and instantiates ActiveRecord models for every record. You want User.find_each do |user|, which uses find_in_batches to load 1000 records at a time.

  2. Don't do simple filtering in Ruby, do it in the database. This...

    if user.reminder == true 

    should be a simple scope that prevents these records from ever being loaded. Something like this:

    class User < ActiveRecord::Base
      scope :with_reminder, -> { where(reminder: true) }

    Now, your top-level loop can use the scope:

    User.with_reminder.find_each do |user|

    This filters out a huge number of users who would otherwise be loaded from the database needlessly.

  3. Your if/else aren't actually correct, since your intent is to load all users with reminder = true. You load all users, and then your if looks for user.reminder == true, but your else doesn't. It affects all users with reminder of true or false, and then sets their reminded to false.

  4. ... which tells me that the user hasn't an ActiveRecord assigned to it? Because the reminder_time column exists and is set (to 16:00).

    Err, no, that's not at all what that tells you. Nothing has "an ActiveRecord" assigned to it, that isn't at all how ActiveRecord works. Every record in the database is wrapped in an ActiveRecord object as you read it from the database, it's impossible for some users to "have" ActiveRecords and others not to.

    See point 3. Your else currently checks all records, including those with reminder == false, and presumably those records don't have a reminder_time set.

  5. It's really not clear what you're trying to do with Time.current.strftime("%H") == user.reminder_time + 1. What is "1"? You have no unit, so it means "1 second". Your probably never going to hit that.

  • \$\begingroup\$ This is great, and exactly what I needed. And you were right about every point. Cleared things up a lot. Thanks a bunch for this! :) \$\endgroup\$
    – Thierry M.
    Commented Feb 11, 2016 at 7:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.