2016-01-25 2 views
0

У меня есть два класса, отвечающих за проверки атрибутов:Уменьшение ненужного дублирования между двумя классами

class NameValidator < ActiveModel::EachValidator 
    def validate_each(record, attribute, value) 
    message = options.fetch(:message, I18n.t('errors.attributes.name.invalid')) 
    record.errors[attribute] << message unless NameValidator.valid_name?(value) 
    end 

    def self.valid_name?(name) 
    name =~ /\A[a-z][\w\p{Blank}]+\z/i 
    end 
end 

и второй

class EmailValidator < ActiveModel::EachValidator 
    def validate_each(record, attribute, value) 
    message = options.fetch(:message, I18n.t('errors.attributes.email.invalid')) 
    record.errors[attribute] << message unless EmailValidator.valid_email?(value) 
    end 

    def self.valid_email?(email) 
    email =~ /\[email protected]+\..+\z/i 
    end 
end 

Они в основном то же самое. Должен ли я наследовать их из одного класса с помощью защищенных методов утилиты или что?

+0

ИМХО это было бы хорошим вопросом для http://codereview.stackexchange.com/ –

ответ

1

Вы можете упростить это далее

class PatternValidator < ActiveModel::EachValidator 
    def validate_each(record, attribute, value) 
    message = options.fetch(:message) || kind 
    record.errors[attribute] << message unless value =~ validation_pattern 
    end 
end 

class NameValidator < PatternValidator 
    def validation_pattern; /\A[a-z][\w\p{Blank}]+\z/i end 
end 

class EmailValidator < PatternValidator 
    def validation_pattern; /\[email protected]+\..+\z/i end 
end 

EachValidator имеет # добрый метод, поэтому он добавит: имя или: email как сообщение об ошибке, если не переопределено. Затем вы можете оставить i18n для поиска в соответствии со стандартным каскадом, как описано в rails guide.

+0

Это красиво, спасибо. –

0

Держите их отдельно для ясности. Методы достаточно малы, где обфускация абстракции сделает его более, не менее очевидным, что происходит.

Если у вас есть 3,4,5,6 или более аналогичных валидаторов, где этот шаблон начинает казаться очевидным, добавление абстракции может облегчить понимание, изменение, зависимость или удаление.

+0

Это то, что в моем проекте есть больше валидаторов. Подтверждение заголовка заголовка, например .. –

+0

Как насчет использования встроенного модуля проверки формата ActiveModel? http://edgeguides.rubyonrails.org/active_record_validations.html#format – Glenn

+0

Нет, я извлек этот валидатор, чтобы избежать такой ситуации. –

1

Используйте наследование только тогда, когда один класс явно является особым случаем другого. В вашем примере, кажется, что два класса равны. Затем используйте mixin, а не наследование.

Небольшой пункт в вашем коде, который стоит против общего использования validate_each является жестким кодом NameValidator.valid_name? и EmailValidator.valid_email?. Вы должны сделать их одинаковыми в общем коде, который будет использоваться в обоих классах. Прежде всего, вам не нужно указывать разные имена valid_name? и valid_email?. Их различия должны быть поглощены использованием соответствующих классов. Используйте общее имя. Во-вторых, вам не нужно жестко закодировать приемник. Вместо этого используйте self.class. Но вместо того, чтобы делать это с помощью методов класса, используйте метод экземпляра.

module ValidatorModule 
    def validate_each(record, attribute, value) 
    message = options.fetch(:message, I18n.t("errors.attributes.#{attribute}.invalid")) 
    record.errors[attribute] << message unless valid?(value) 
    end 
end 

class NameValidator < ActiveModel::EachValidator 
    include ValidatorModule 
    def attribute; "name" end 
    def valid?(value); value =~ /\A[a-z][\w\p{Blank}]+\z/i end 
end 

class EmailValidator < ActiveModel::EachValidator 
    include ValidatorModule 
    def attribute; "email" end 
    def valid?(value); value =~ /\[email protected]+\..+\z/i end 
end 

Если вы считаете, что проверка всегда делается с помощью одного регулярного выражения, вы можете пойти один шаг дальше:

module ValidatorModule 
    def validate_each(record, attribute, value) 
    message = options.fetch(:message, I18n.t("errors.attributes.#{attribute}.invalid")) 
    record.errors[attribute] << message unless value =~ validation_pattern 
    end 
end 

class NameValidator < ActiveModel::EachValidator 
    include ValidatorModule 
    def attribute; "name" end 
    def validation_pattern; /\A[a-z][\w\p{Blank}]+\z/i end 
end 

class EmailValidator < ActiveModel::EachValidator 
    include ValidatorModule 
    def attribute; "email" end 
    def validation_pattern; /\[email protected]+\..+\z/i end 
end 
Смежные вопросы