Skip to main content
added 39 characters in body
Source Link
tokland
  • 11.1k
  • 1
  • 20
  • 25

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

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:

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

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
Source Link
tokland
  • 11.1k
  • 1
  • 20
  • 25

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:

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