Это правильное применение ПСП (принцип единой ответственности)? - PullRequest
2 голосов
/ 13 июня 2019

У меня есть класс Java:

class User {
 private String name;
 private String address;
 private int age;
 private BigDecimal salary;
 // other fields
 //getters setters
}

Я могу получить карту новых значений в этих полях и обновить ее.Это выглядит так: ChangeItem changeItem, где changeItem.key - это имя поля , а changeItem.value - это значение поля

Я создаю стратегии для обновления каждого поля.Например, общий интерфейс:

public interface UpdateStrategy<T> {
    T updateField(T t, ChangeItem changeItem) throws ValidationExceptions;
}

И некоторые реализации:

public class UpdateNameStrategy implements UpdateStrategy<User> {

    private static final Pattern USER_NAME = Pattern.compile(...);

    @Override
    public User updateField(User user, ChangeItem changeItem) throws ValidationExceptions {

        String fieldValue = changeItem.value;

        if (!validate(fieldValue))
            throw new ValidationExceptions(changeItem);

        user.setName(fieldValue);

        return user;
    }

    private boolean validate(String value){
        return USER_NAME.matcher(value).matches();
     }
}

В реальном проекте у меня есть 40 полей и 40 стратегий для каждого поля (с разной проверкой и логикой).

Я думаю, что этот класс нарушает SRP (принцип единой ответственности).И я перемещаю логику проверки в отдельный класс.Я изменил метод проверки на:

public class UpdateNameStrategy implements UpdateStrategy<User> {

    @Override
    public User updateField(User user, ChangeItem changeItem) throws ValidationExceptions {

        String fieldValue = changeItem.value;

        ValidateFieldStrategy fieldValidator = new UserNameValidate(fieldValue);

        if (!fieldValidator.validate())
            throw new ValidationExceptions(changeItem);

        return user;
    }
}

и

public class UserNameValidate implements ValidateFieldStrategy {

    private static final Pattern USER_NAME = Pattern.compile(...);

    private String value;

    public UserNameValidate(String value) {
        this.value = value;
    }

    @Override
    public boolean validate() {
        return USER_NAME.matcher(value).matches();
    }
}

И теперь у меня есть 40 стратегий для полей обновления и 40 валидаторов.Это правильный путь?Или, может быть, я могу изменить этот код более ясно?

1 Ответ

2 голосов
/ 13 июня 2019

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

Не зная ничего специфического для вашей проблемы домена, это выглядит как излишнее использование шаблона Стратегии.

Я никогда не сталкивался с проблемой допустимого домена, требующей разделения стратегии проверки, подобной этой, для каждого отдельного поля.

Объект в домене - это не просто набор полей.Это также поведение, управляющее полями (которое является состоянием объекта) и правилами, регулирующими изменчивость этого состояния.

В общем, мы хотим иметь богатые объекты с поведением.И это поведение обычно включает в себя проверку.

Я искренне сомневаюсь, что каждое отдельное поле в модели требует проверки до этого уровня детализации.Поместите валидацию в методы установки объекта и покончите с этим.

Вы убиваете себя, делая все эти сложные настройки.Нам всем нужна структура, но в какой-то момент все это просто церемония строительства очень высоких песчаных замков.

Проверка в целом является частью объекта.И объект ответственен, он отвечает за управление своим состоянием, коллекцией полей и значений, которыми он владеет и контролирует.

Принцип единой ответственности не означает освобождение от ответственности проверки полей из объекта.Эта ответственность присуща объекту.

Принцип единой ответственности касается «внешней» ответственности, ответственности объекта за предоставление единой связной функции (или набора связных функций) тому, кто использует этот объект.

Рассмотрим объект принтера.Этот объект отвечает за печать.Например, он не отвечает за управление сетевыми соединениями между принтером и пользователем.

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

Вот что такое SRP.То, что вы делаете, извлекает поведение проверки из объекта, который имеет мало, если вообще что-то, имеет отношение к SRP.

Иногда может потребоваться извлечь обычные процедуры проверки (проверьте, является ли строка черной или нулевой)или является ли число натуральным числом.)

Таким образом, у вас может быть класс, подобный следующему:

public class User {
    // some fields, blah blah

    public void setName(final String aName){
        if( aName == null || a.aName.trim().length() < 1){
            throw new SomeException("empty string blah blah");
        }
        this.name=aName.trim(); // model requires this to be trimmed.
    }

    public void setRawField(final String aValue){
        if( aName == null || a.aName.trim().length() < 1){
            throw new SomeException("empty string blah blah");
        }
        this.rawField=aValue;  // model requires this to not be trimmed.
    }

public void setRawField2(final String aValue){
    // model requires this field to be non-null,
    // can be blank, and if not blank, must be all lower case.
    if(aValue == null) {
        throw new NullPointerException("null string blah blah");
    }
    this.rawField2=aValue.toLowerCase();

}

заменен на класс, который делегирует мелочи к вспомогательному классу или модулю внешней проверки..

public class User {
     // some fields, blah blah

     public void setName(final String aName){
         // model requires this to be trimmed
         this.name=Validator.notEmptyOrDie(aName).trim();
     }

     public void setRawField(final String  aValue){
         // model requires this to *not* be trimmed
         this.rawField=Validator.notEmptyOrDie(aValue);
     }

public void setRawField2(final String aValue){
    // model requires this field to be non-null,
    // can be blank, and if not blank, must be all lower case.
    // too contrive to refactor, leave it here.
    if(aValue == null) {
        throw new NullPointerException("null string blah blah");
    }
    this.rawField2=aValue.toLowerCase();

}

 public class Validator {
     static public String notEmptyOrDie(final String aString){
         if( aString == null || aString.trim().length() < 1){
             throw new SomeException("empty string blah blah");
         }
         return aString;
 }

Я придерживаюсь этого подхода к рефакторингу частей общей проверки.Я исключаю мелочи.

Но основная логика проверки, если таковая имеется, остается в объекте. Обратите внимание, что проверка все еще является частью класса User.Все, что было извлечено, это мелочи.

Логика, которая объявляет намерение проверки ( проверка, черный или умерший ) все еще остается частью класса User.Это присуще поведению класса.

В некоторых моделях класс User может вообще не требовать проверки.Это может быть просто передача данных, POJO.

OTH, в модели, которая требует, чтобы оно проверяло свое состояние, это состояние обычно должно входить в класс, и у разработчика должен быть очень хороший аргумент для удаления этой логики, как вы делали это в своем примере кода.

SRP ничего не говорит о том, как вы возлагаете ответственность внутри объекта на внешние, а только на внешние стороны этого объекта.

Как правило, проверка полей объекта принадлежит объекту как внутренняя логикак объекту.Он присущ поведению объекта, его инвариантам, предусловиям и постусловиям.

Очень редко вы извлекаете всю проверку из объекта (если мы не говорим о POJO, сериализованных и десериализованных внешним пакетом, и с проверками, добавленными декларативно через аннотации или некоторый управляющий дескриптор конфигурации.)

Ударь меня, если у тебя еще остались вопросы. Не знаю, как быстро я смогу ответить, но я не против ответить на вопросы, если смогу.

**** РЕДАКТИРОВАТЬ ***

Пользователь @qujck упоминает действительную проблему в этом предлагаемом подходе, что невозможно дифференцировать все исключения валидации (потому что они используют общие исключения для всех.)

Одна возможность (которую я использовал) состоит в том, чтобы иметь перегруженные и / или полиморфные валидаторы:

public class Validator {
    static public String notEmptyOrDie(final String aString){
        return Validator.notEmptyOrDie(aString, null);
    }

    static public String notEmptyOrDie(final String aString, 
                                       final String aFieldName){
        if( aString == null || aString.trim().length() < 1){
            throw new SomeException( 
                (aFieldName==null? "" : aFieldName + " ") 
                + "empty string blah blah");
        }
        return aString;
    }
}

Если использовать иерархию исключений проверки с общими конструкторами, то можно пойти дальше, передав желаемый класс исключений, и использовать отражение для создания экземпляров, которые будут выброшены.

Я тоже это сделал. На самом деле, я делаю это сейчас для общего механизма генерирования ошибок на уровне EJB, который сам достигает другой системы через сетевые протоколы.

Но это то, что я делаю, чтобы справиться с существующей системой, а не то, что я сделал бы, если бы у меня был выбор дизайна. И это все еще ограничивается рефакторингом валидации или обработки ошибок для его основных элементов.

Фактическая, специфичная для объекта валидация все еще остается в / в самого объекта.

...