Sidekiq worker to validate survey participants

Question

I have worker code that looks like this:

class VerificationWorker
  include Sidekiq::Worker

  def perform(panel_id)
    members = MembersAPI.members.fetch

    members.each do |member|
      survey = Survey.find_by(number: member.data_number)
      next if survey.nil?

      if survey.id != member.data['survey_id']
        survey.fail!('survey_id does not match')
        next
      end

      survey.recipient_id = member.id if survey.recipient_id.blank?
      survey.recipient_id == member.id ? survey.verify! : survey.fail_panel!('recipient_id does not match member id')
    end
  end
end

What do you think about this code? Is it good enough?

Answer 1

Your code looks pretty good to me. Just some minor notes:

• next if survey.nil? -> next unless survey is enough.
• The conditional operator ? is normally used for expressions, not for actions with side-effects. Also, it's a long line, a normal if-else would look better.
• Argument panel_id is not used?

That said, I'd refactor it a little bit. In my experience, full-fledged conditionals (no inline conditionals) make code easier to read so the line count increase is worth it.

class VerificationWorker
  include Sidekiq::Worker

  def perform(panel_id)
    members = MembersAPI.members.fetch

    members.each do |member|
      if (survey = Survey.find_by(number: member.data_number))
        if survey.id != member.data['survey_id']
          survey.fail!('survey_id does not match')
        else
          survey.recipient_id = survey.recipient_id.presence || member.id

          if survey.recipient_id == member.id
            survey.verify!
          else
            survey.fail_panel!('recipient_id does not match member id')
          end
        end
      end
    end
  end
end

Answer 2

You could refactor this code a bit

class VerificationWorker
  include Sidekiq::Worker

  def perform(panel_id)
    members = MembersAPI.members.fetch

    members.each do |member|
      fetch_survey_for(member) do |survey|
        set_survey_recipient survey, member                              
      end
    end
  end

private

  def fetch_survey_for member
    if survey = Survey.find_by(number: member.data_number)
      if survey.id != member.data['survey_id']
        survey.fail! I18n.t('survey.errors.id_mismatch')
      else
        yield survey
      end
    end
  end

  def set_survey_recipient survey, member
    survey.recipient_id ||= member.id    
    if survey.recipient_id == member.id
      survey.verify!
    else
      survey.fail_panel! I18n.t('survey.errors.recipient_id_mismatch')
    end  
  end      
end

Create config/locals/survey.yml add your messages, for easier changes. Inside add

en:
  survey:
    errors:
      id_mismatch: "survey_id does not match"
      recipient_id_mismatch: "recipient_id does not match member id"