Как возразить против такого «абсолютно публичного» мышления при проектировании классов бизнес-объектов?

9

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

Пример класса, который я не фанат:

public class Foo
{
    private string field1;
    private string field2;
    private string field3;
    private string field4;
    private string field5;

    public Foo() { }

    public Foo(string in1, string in2)
    {
        field1 = in1;
        field2 = in2;
    }

    public Foo(string in1, string in2, string in3, string in4)
    {
        field1 = in1;
        field2 = in2;
        field3 = in3;
    }

    public Prop1
    { get { return field1; } }
    { set { field1 = value; } }

    public Prop2
    { get { return field2; } }
    { set { field2 = value; } }

    public Prop3
    { get { return field3; } }
    { set { field3 = value; } }

    public Prop4
    { get { return field4; } }
    { set { field4 = value; } }

    public Prop5
    { get { return field5; } }
    { set { field5 = value; } }

}

В «реальном» классе они не все строковые, но в некоторых случаях у нас есть 30 вспомогательных полей для полностью открытых свойств.

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

  • Частные поля поддержки без логики в свойствах, кажутся ненужными и раздувают класс
  • Несколько конструкторов (несколько хорошо), но в сочетании с
  • все свойства, имеющие открытый сеттер, я не фанат.
  • Потенциально никаким свойствам не будет присвоено значение из-за пустого конструктора, если вызывающий объект не знает, вы можете получить некоторые очень нежелательные и сложные для проверки поведения.
  • Это слишком много свойств! (в 30 случае)

Я нахожу гораздо сложнее действительно понять, в каком состоянии находится объект Fooв любой момент времени, как реализатор. Был выдвинут аргумент: «У нас может не быть необходимой информации для установки Prop5во время создания объекта. Хорошо, я думаю, я могу это понять, но если это так, сделайте Prop5общедоступным только сеттер, а не всегда до 30 свойств в классе.

Я просто придирчив и / или сумасшедший из-за того, что мне нужен класс, который «прост в использовании», а не «легко писать (все публично)»? Классы, подобные приведенным выше, кричат ​​мне, я не знаю, как это будет использоваться, поэтому я просто собираюсь обнародовать все на всякий случай .

Если я не очень требователен, каковы хорошие аргументы для борьбы с этим типом мышления? Я не очень хорош в формулировании аргументов, потому что я очень расстроен, пытаясь донести свою точку зрения (не намеренно, конечно).

Kritner
источник
3
Объект всегда должен быть в допустимом состоянии; то есть вам не нужно иметь частично инстанцированные объекты . Некоторая информация не является частью основного состояния объекта, в то время как другая информация (например, таблица должна иметь ножки, но ткань не обязательна); дополнительная информация может быть предоставлена ​​после создания экземпляра, основная информация должна быть предоставлена ​​при создании экземпляра. Если некоторая информация является основной, но не известна при создании экземпляра, то я бы сказал, что классы нуждаются в рефакторинге. Трудно сказать больше, не зная контекста класса.
Марвин
1
Сказав, что «у нас может не быть необходимой информации для установки Prop5 во время строительства объекта», я спрашиваю: «Вы когда-нибудь говорили, что у вас будет эта информация во время строительства, а в другие моменты, когда вы этого не сделаете? есть эта информация? Это заставляет меня задуматься, существуют ли на самом деле две разные вещи, которые этот один класс пытается представить, или, возможно, этот класс следует разбить на более мелкие классы. Но если это сделано «на всякий случай», это тоже плохо, ИМО; это говорит мне, что не существует четкого дизайна для того, как объекты создаются / заполняются.
Ученик доктора Вилли
3
Для частных полей поддержки без логики в свойствах, есть ли причина, по которой вы не используете авто свойства ?
Carson63000
2
@Kritner весь смысл авто-свойств в том, что если вам в будущем понадобится настоящая логика, вы можете легко добавить ее вместо авто getили set:-)
Carson63000

Ответы:

10

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

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

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

Для решения других ваших вопросов:

  • «частные поля поддержки без логики в свойствах»: Да, вы правы, для тривиальных методов получения и установки это просто ненужный «шум». Чтобы избежать такого типа «раздувания», C # имеет сокращенный синтаксис для методов get / set свойства:

Так что вместо

   private string field1;
   public string Prop1
   { get { return field1; } }
   { set { field1 = value; } }

записывать

   public string Prop1 { get;set;}

или

   public string Prop1 { get;private set;}
  • «Несколько конструкторов»: это не проблема сама по себе. Это становится проблемой, когда возникает ненужное дублирование кода, как показано в вашем примере, или когда иерархия вызовов запутана. Это может быть легко решено путем рефакторинга общих частей в отдельную функцию и путем организации цепочки конструктора однонаправленным образом.

  • «Потенциально никаким свойствам не будет присвоено значение из-за пустого конструктора»: в C # каждый тип данных имеет четко определенное значение по умолчанию. Если свойства не инициализируются явно в конструкторе, им присваивается это значение по умолчанию. Если это используется намеренно , это совершенно нормально, поэтому пустой конструктор может быть в порядке, если автор знает, что он делает.

  • «Это слишком много свойств! (В случае с 30)»: да, если вы можете разработать такой класс с нуля, 30 - это слишком много, я согласен. Однако не у каждого из нас есть такая роскошь (разве вы не написали в комментарии ниже, что это устаревшая система?). Иногда вам необходимо отобразить записи из существующей базы данных, файла или данных стороннего API в вашу систему. Таким образом, для этих случаев 30 атрибутов могут быть чем-то, с чем нужно жить.

Док Браун
источник
Спасибо, да, я знаю, что POCO / POJO имеют свое место, и мой пример довольно абстрактный. Однако я не думаю, что смогу конкретизировать, не вдаваясь в роман.
Критнер
@Kritner: см. Мое редактирование
Док Браун
да, обычно, когда я создаю новый класс, я прохожу автопортрет. Наличие частных полей поддержки «только потому, что» (на мой взгляд) не требуется и даже вызывает проблемы. Когда у вас есть ~ 30 свойств для класса и более 100 классов, нет ничего плохого в том, чтобы сказать, что будут какие-то свойства или настройки из неправильного поля поддержки ... Я уже сталкивался с этим несколько раз в нашем рефакторинге. :)
Kritner
спасибо, значение по умолчанию для строки nullэто не так? так что если одно из свойств, которое фактически используется в a .ToUpper(), но для него никогда не задано значение, это вызовет исключение времени выполнения. Это еще один хороший пример , почему необходимые данные для класса должны иметь быть установлены во время строительства объекта. Не просто оставить это на усмотрение пользователя. Спасибо
Критнер
Помимо слишком большого числа неустановленных свойств, основная проблема этого класса заключается в том, что он имеет нулевую логику (ZRP) и является архетипом для анемической модели. Без необходимости в этом, которая может быть выражена словами, например, DTO, это плохой дизайн.
user949300
2
  1. Говоря, что «Частные вспомогательные поля без логики в свойствах кажутся ненужными и раздувают класс», у вас уже есть достойный аргумент.

  2. Не похоже, что несколько конструкторов являются проблемой здесь.

  3. Что касается того, чтобы все свойства были общедоступными ... Возможно, подумайте об этом так, если вы когда-нибудь захотите синхронизировать доступ ко всем этим свойствам между потоками, вам будет непросто, потому что вам, возможно, придется писать код синхронизации везде, где они используемый. Однако, если бы все свойства были заключены в getter / setters, вы могли бы легко встроить синхронизацию в класс.

  4. Я думаю, что когда вы говорите «трудно проверить поведение», аргумент говорит сам за себя. Ваш тест, возможно, должен проверить, не является ли он нулевым или что-то в этом роде (каждое отдельное место, где используется свойство). Я действительно не знаю, потому что я не знаю, как выглядит ваш тест / приложение.

  5. Вы правы слишком много свойств, не могли бы вы использовать наследование (если свойства достаточно похожи), а затем получить список / массив с использованием обобщений? Тогда вы могли бы написать методы получения / установки для доступа к информации и упростить взаимодействие со всеми свойствами.

шпионить
источник
1
Спасибо - № 1 и № 4, я определенно чувствую себя наиболее комфортно с аргументами. Не совсем уверен, что вы имеете в виду в # 3. # 5 большинство свойств в классах составляют первичный ключ в БД. Мы используем конкурентные ключи, иногда до 8 столбцов - но это другая проблема. Я думал о том, чтобы попытаться поместить части, составляющие ключ, в свой собственный класс / структуру, что исключило бы множество свойств (по крайней мере, в этом классе), но это унаследованное приложение с тысячами строк кода с несколькими звонящие. Так что, думаю, мне есть над чем подумать :)
Kritner
Эх. Если бы класс был неизменным, не было бы никакой причины писать какой-либо код синхронизации внутри класса.
RubberDuck
@RubberDuck Является ли этот класс неизменным? Что вы имеете в виду?
Снуп
3
Это определенно не неизменный @StevieV. Все классы с установщиками свойств являются изменяемыми.
RubberDuck
1
@Kritner Как свойства используются в качестве первичных ключей? Это предположительно требует некоторой логики (нулевые проверки) или правил (например, NAME имеет приоритет над AGE). Согласно ООП этот код принадлежит к этому классу. Не могли бы вы использовать это в качестве аргумента?
user949300
2

Как вы мне сказали, Fooэто бизнес-объект. Он должен инкапсулировать некоторое поведение в методах в соответствии с вашим доменом, а его данные должны оставаться максимально инкапсулированными.

В примере, который вы показываете, он Fooбольше похож на DTO и идет вразрез со всеми принципами ООП (см. Модель анемичного домена )!

В качестве улучшений я бы предложил:

Это может быть потенциальный рефакторинг класса с этими замечаниями:

public sealed class Foo
{
    private readonly string field1;
    private readonly string field2;
    private readonly string field3;
    private readonly string field4;

    public Foo() : this("default1", "default2")
    { }

    public Foo(string in1, string in2) : this(in1, in2, "default3", "default4")
    { }

    public Foo(string in1, string in2, string in3, string in4)
    {
        field1 = in1;
        field2 = in2;
        field3 = in3;
        field4 = in4;
    }

    //Methods with business logic

    //Really needed ?
    public Prop1
    { get; }

    //Really needed ?
    public Prop2
    { get; }

    //Really needed ?
    public Prop3
    { get; }

    //Really needed ?
    public Prop4
    { get; }

    //Really needed ?
    public Prop5
    { get; }
}
пятнистый
источник
2

Как возразить против такого «абсолютно публичного» мышления при проектировании классов бизнес-объектов?

  • «В клиентском классе (ах) есть несколько методов, которые выполняют не просто« обязанности DTO ». Они принадлежат друг другу».
  • «Это может быть« DTO », но у него есть деловая идентичность - нам нужно переопределить Equals».
  • «Нам нужно разобраться в этом - нам нужно реализовать IComparable»
  • «Мы связываем вместе несколько из этих свойств. Давайте переопределим ToString, чтобы каждому клиенту не приходилось писать этот код».
  • «У нас несколько клиентов делают одно и то же с этим классом. Нам нужно высушить код».
  • «Клиент манипулирует коллекцией этих вещей. Очевидно, что это должен быть пользовательский класс коллекции; и многие более ранние пункты сделают эту пользовательскую коллекцию более функциональной, чем в настоящее время».
  • «Посмотрите на все утомительные, открытые манипуляции со строками! Инкапсуляция этого в бизнес-релевантных методах увеличит нашу производительность кодирования».
  • «Объединение этих методов в классе сделает их тестируемыми, потому что нам не нужно иметь дело с более сложным клиентским классом».
  • «Теперь мы можем выполнить модульное тестирование этих рефакторинговых методов. Так как по причинам x, y, z клиент не тестируется.

  • В той степени, в которой любой из приведенных выше аргументов может быть приведен в совокупности:

    • функциональность становится пригодными для повторного использования.
    • Это сухой
    • Это отделено от клиентов.
    • кодирование против класса быстрее и менее подвержено ошибкам.
  • «Хорошо сделанный класс скрывает состояние и предоставляет функциональность. Это стартовое предложение для разработки ОО-классов».
  • «Конечный результат намного лучше выражает бизнес-область; отдельные объекты и их явное взаимодействие».
  • «Эта« накладная »функциональность (то есть не явные функциональные требования, но это должно быть выполнено) явно выделяется и придерживается принципа единой ответственности.
radarbob
источник