Передача объекта в метод, который изменяет объект, является ли это обычным (анти) шаблоном?

Я читаю об общих запахах кода в книге Рефакторинга Мартина Фаулера . В этом контексте мне было интересно узнать о паттерне, который я вижу в кодовой базе, и можно ли объективно рассматривать его как анти-паттерн.

Это шаблон, в котором объект передается в качестве аргумента одному или нескольким методам, каждый из которых изменяет состояние объекта, но ни один из которых не возвращает объект. Таким образом, он полагается на передачу по ссылочной природе (в данном случае) C # /. NET.

var something = new Thing();
// ...
Foo(something);
int result = Bar(something, 42);
Baz(something);

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

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

var something1 =  new Thing();
// ...

// Let's return a new instance of Thing
var something2 = Foo(something1);

// Let's use out param to 'return' other info about the operation
int result;
var something3 = Bar(something2, out result);

// If necessary, let's capture and make explicit complex changes
var changes = Baz(something3)
something3.Apply(changes);

Мне кажется, первый шаблон выбран на предположениях

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

Я иллюстрирую альтернативу, но чтобы предложить ее, нужно иметь аргументы против первоначального решения. Какие, если таковые имеются, аргументы могут быть приведены, чтобы доказать, что исходное решение является анти-паттерном?

А что, если вообще что-то не так с моим альтернативным решением?

Да, исходное решение является анти-паттерном по причинам, которые вы описываете: оно затрудняет рассуждение о том, что происходит, объект не отвечает за свое собственное состояние / реализацию (нарушение инкапсуляции). Я также добавил бы, что все эти изменения состояния являются неявными контрактами метода, что делает этот метод хрупким перед лицом меняющихся требований.

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

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

когда методы не названы соответствующим образом

На самом деле, это настоящий запах кода. Если у вас есть изменяемый объект, он предоставляет методы для изменения его состояния. Если у вас есть вызов к такому методу, встроенному в задачу еще нескольких операторов, то вполне можно реорганизовать эту задачу в собственный метод, что в точности описывает описанную ситуацию. Но если у вас нет имен методов, таких как Fooи Bar, но есть методы, которые дают понять, что они изменяют объект, я не вижу здесь проблемы. Думать о

void AddMessageToLog(Logger logger, string msg)
{
    //...
}

или

void StripInvalidCharsFromName(Person p)
{
// ...
}

или

void AddValueToRepo(Repository repo,int val)
{
// ...
}

или

void TransferMoneyBetweenAccounts(Account source, Account destination, decimal amount)
{
// ...
}

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

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

Да, см. Http://codebetter.com/matthewpodwysocki/2008/04/30/side-effecting-functions-are-code-smells/ для одного из многих примеров людей, указывающих на то, что неожиданные побочные эффекты являются плохими.

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

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

Во-вторых, является ли это проблемой или нет, зависит как от объекта, так и от того, насколько тесно связаны изменения в разных процедурах - если вы не можете внести изменения в Foo и это приводит к сбою Bar, то это проблема. Не обязательно пахнет кодом, но это проблема либо с Foo, либо с Bar, либо с Something (возможно, Bar, поскольку он должен проверять свой ввод, но это может быть что-то, переводимое в недопустимое состояние, которое должно быть предотвращено).

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

Я бы сказал, что есть небольшая разница между A.Do(Something)модификацией somethingи something.Do()модификацией something. В любом случае это должно быть ясно из имени вызванного метода, который somethingбудет изменен. Если это не ясно из имени метода, независимо от того, somethingявляется ли он параметром thisили частью среды, его не следует изменять.

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

var users = Dependency.Resolve<IGetUsersQuery>().GetAll();

var excludeAdminUsersFilter = new ExcludeAdminUsersFilter();
var filterByAnotherCriteria = new AnotherCriteriaFilter();

excludeAdminUsersFilter.Apply(users);
filterByAnotherCriteria.Apply(users); 

И да, вы можете сделать это красиво, переместив фильтрацию в другой метод, так что вы получите что-то вроде:

var users = Dependency.Resolve<IGetUsersQuery>().GetAll();
Filter(users);

Где Filter(users)бы выполнить вышеупомянутые фильтры.

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

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

Допустим, я пишу сложную математическую операцию как функцию f () . Я документирую, что f () - это функция, которая сопоставляется NXNс Nалгоритмом. Если функция названа ненадлежащим образом и не документирована, и в ней нет сопровождающих тестовых примеров, вам придется разбираться в коде, и в этом случае код бесполезен.

О вашем решении, некоторые замечания:

• Приложения разрабатываются с разных сторон: когда объект используется только для хранения значений или передается через границы компонентов, разумно изменить внутреннюю часть объекта внешне, а не заполнять его сведениями о том, как его изменить.
• Клонирование объектов приводит к раздутию требований к памяти и во многих случаях приводит к существованию эквивалентных объектов в несовместимых состояниях ( Xстало Yпосле f(), но Xфактически Y) и, возможно, временной несогласованности.

Проблема, которую вы пытаетесь решить, является действительной; однако даже после огромного перепроектирования проблема обойдена, а не решена.