1
\$\begingroup\$
class InvitationConfirmationValidator < ActiveModel::Validator
  def validate(record)
    invitation = Invitation.find_by_url_suffix(record.id)
    if invitation
      if invitation.materialized?
        record.erros.clear
        record.errors[:base] << "Invitation already materialized"
      end
    else
      record.erros.clear
      record.errors[:base] << "Invitation does not exist"
    end
  end
end

The above is the code for a Validator. It first checks if an invitation with an ID exists. If it does, it checks whether it has already been used.

The code seems to me more complex than it should be to achieve this simple task. Can its complexity be reduced?

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

The first thing I would do is assign those strings to constants since they will never change throughout the life of the program. Also, I'm not sure why you have to clear the errors out when you validate this part of the model. When I run valid? on a model, personally I would like to see ALL of the reasons that the model isn't valid.

class InvitationConfirmationValidator < ActiveModel::Validator
  def validate(record)
    invitation = Invitation.find_by_url_suffix(record.id)
    return record.errors[:base] << MISSING_INVITE_MESSAGE unless invitation
    return record.errors[:base] << MATERIALIZED_INVITE_MESSAGE if invitation.materialized?
  end
end
\$\endgroup\$
1
\$\begingroup\$

@arjabbar's has an excellent answer, I just want to add an observation regarding names:

invitation = Invitation.find_by_url_suffix(record.id)

Is record.id a URL suffix? Is the url_suffix column in Invitation an ID?

Neither option seems plausible, so it might be a naming problem - name the fields appropriately to what they hold:

invitation = Invitation.find_by_record_id(record.id)
\$\endgroup\$

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.