Некоторое время назад, вернувшись после полугодового отпуска в функциональном мире, назад в ООП, я в который раз наступил на привычные грабли: случайно изменил состояние.
private double fBm(Vector2D v, int y)
{
double result = 0f;
double freq = Frequency;
for (int i = 0; i < Octaves; ++i)
{
result += NoiseFn(permutation, v * freq) * Amplitude;
freq *= Lacunarity;
Amplitude *= Gain; // <-- Вот тут.
}
return result;
}В ФП нужно особо постараться чтобы получить такой баг, а в некоторый языках невозможно в принципе. Салат из полезной работы и состояния класса не радовал, простор для ошибок даже в этой четверке строк слишком широк. Я стал думать как можно уменьшить площадь этих грабель и вывел следующее:
Во-первых нужно сделать this обязательным. Поставить поле не с той стороны выражения слишком легко. Если бы я везде явно указал контекст, скорее всего сразу бы заметил, что ступил. К сожалению, компилятор C# нельзя заставить требовать this, а в статических проверках студии есть только правило которое работает наоборот, то есть заставляет убирать this. Так что, видимо придется писать свое.
Во-вторых нужно исключить использование полей класса из тела метода, а все изменения записывать в конце метода, если они есть. У нас получается три абзаца. В первом мы объявляем все что нам понадобится, во втором делаем полезную работу, в третьем сохраняем состояние.
public void DoAThing(int input)
{
// Первый абзац
int a = this.A;
int b = this.B;
int result = 0;
// Второй абзац
for (int i = 0; i < input; ++i)
result += (a + b) * i;
// Третий абзац
this.Thing = result;
}В первом абзаце все состояние которое нужно мы сохраняем локально.
Во втором, this не фигурирует.
В третьем this всегда должен быть первым словом. Тут же будут вызываться другие методы которые меняют состояние.
return можно добавить в любое место, на поведение метода он не повлияет.
Плюсы
- Багов со случайным изменением состояния станет меньше.
- Отсутствие третьего абзаца делает метод чистой функцией, а значит его можно запускать параллельно.
Минусы
- Дисциплина. Фу-фу-фу. Без автоматических проверок, легко все испортить.
- В некоторых случаях этот подход будет работать медленней.
- Может выглядеть странно или глупо. Вот к примеру:
{
int a = this.A;
int b = this.B;
return a + b;
}Вместо заключения
Как думаете? В этом есть смысл или я парюсь? Если вы уже делаете подобное или натягиваете другие части ФП на ООП, пожалуйста поделитесь.
Комментарии (34)
maaGames
02.01.2017 17:08Поздравляю, вы изобрели «транзакции»!
Вообще, это довольно хорошая практика программирования, исключающая порчу состояния объекта при экстренном завершении метода. Но он снижает перформанс, так что надо выбирать.
Сразу дам совет. Гораздо проще и удобнее создавать не отдельные поля, а целую копию объекта в «абзац 1» и присваивать копию оригиналу в «абзац 3». Тогда вам безразлично наличие/отсутствие this, т.к. в левой части у вас должен быть модифицируемый объект (который копия). Думаю, говорить о бессбойном конструкторе копирования и присваивания даже упоминать не обязательно.
aikixd
02.01.2017 17:56Как можно присвоить объект оригиналу?
this = ...сработает только в структуре, если мы говорим про шарп.maaGames
02.01.2017 18:19В C# так нельзя? Ну, тогда метод Copy наверняка сделать можно.
В С++ без проблем можно *this = a; написать. Удобно, пока не отстрелишь что-нибудь…aikixd
02.01.2017 18:43Нельзя. Можно сделать так:
public void MakeThing(Something ref obj) { var thing = new Something(obj); // ... obj = thing; }
Но в шарпе фокусы с ссылками и особенно оказателями используются только в крайнем случае. Для указателей даже компилятору нужно сказать, что если что — сам дурак.
sashagil
02.01.2017 23:03В C++ есть идиома с вызовом Swap в конце метода, хорошо сочетается с идиомой pImpl (данные объекта хранятся в отдельном блоке — «реализации», а сам объект — «фасад» — обёртка над смартпойнером на этот блок). Дополнительные бонусы — простая дисциплина потоковой безопасности (надо позаботиться только о безопасности Swap) и сильная безопасность по исключениями (объект никогда не остаётся в некорректном «смешанном» состоянии, если посреди метода происходит исключение, при этом требование noexcept необходимо только для Swap). Эта идиома полуофициально поддерживается в STL (см. реализацию stl::swap для классов библиотеки плюс для POD типов).
При использовании этой идиомы дисциплину можно организовать такую. Объекты-реализации никогда не имеют методов, меняющих состояние, но, зато, имеют достаточно богатые наборы конструкторов. Основное тело метода объекта public API (фасада соответствующего объекта-реализации) состоит в построении новой версии объекта-реализации (при этом копировать поля текущего объекта в локальные переменные, как у автора поста, смысла нет — компилятор в помощь, поскольку pImpl-смартпойнтер фасада указывает на константный объект, да и все методы объекта-реализации помечены const). Эта часть метода завершается вызовом конструктора нового объекта-реализации, с сохранением указателя на него в локальную переменную — такой же смартпойнтер, как pImpl. Заключительный оператор метода — swap (std::swap, для единообразия) этой переменной и единственного поля фасада — pImpl.
Кстати, роль явного this (как у автора поста) здесь играет необходимость обращаться к реализации через единственное поле фасада — pImpl.
Дополнительная стоимость такого подхода, применяемого строго, понятна — но в каких-то применениях она оправдана, а узкие по производительности места можно и подрихтовать, если надо.
qw1
03.01.2017 00:16Я бы не стал предлагать этот подход как универсальное решение потокобезопасности. Segfault из-за конкурентного доступа мы так не получим, но вот логика приложения может пострадать. Всё-таки, не зря классический подход — это Lock на всё время работы функции, выполняющей изменение данных.
sashagil
03.01.2017 00:30Согласен, но это один из разумных компромиссов для тех случаев, когда Lock в каждом модифицирующем методе — дорог по производительности. Бескомпромисный вариант — делать все структуры данных, разделяемые между потоками, immutable, с единственным корнем, который обновляется atomic операцией. Но это тоже дорого иногда (из-за дороговизны immutable версий сложных структур данных).
qw1
03.01.2017 09:58Это тоже не выход. Вот дерево:
[корень]
|>[Нода1] Значение:50
|>[Нода2] Значение:30
Если два потока параллельно хотят увеличить значение в ноде1 на 10 и 20 соответственно, то переприсвоение корня оставит результат только одной операции, а вторая потеряется.
Есть, конечно, атомарный compare-and-swap (CAS). Это когда мы заранее запоминает указатель на корень, а при замене сравниваем с прошлым значением и, если корень изменился, swap не делаем, а откатываем всё операцию и начинаем заново.
Но CAS на весь корень это очень дорого (т.к. по сути успешно завершится модификация только из одного потока, а остальные будут впустую жечь электричество). Тут честный Lock на всё время операции экономнее.wheercool
03.01.2017 22:48Но CAS на весь корень это очень дорого (т.к. по сути успешно завершится модификация только из одного потока, а остальные будут впустую жечь электричество). Тут честный Lock на всё время операции экономнее.
В этом и суть CAS операций и не важно корень это или операции мутации по отдельности. В качестве бонуса мы получаем lock-free реализацию, которую получить на lock-ах по определению нельзя.qw1
03.01.2017 23:09CAS на корень — это гарантированный худший сценарий, потому как любое параллельное действие сделает откат всего, что сделано в последней транзакции, без шансов немного приостановиться и подождать.
Цель какая — эффективность или любым способом заюзать модный lock-free?
Оптимистичные алгоритмы (CAS) хороши, когда конфликт редко случается. Пессимистичные (lock) — когда часто. У каждого своя область применения.
И главное, на CAS не будет честной очереди. Длинная операция всегда будет откатываться, если не прекращаются короткие запросы. Lock ставит всех в одну очередь.
qw1
03.01.2017 23:19В этом и суть CAS операций и не важно корень это или операции мутации по отдельности.
Как это не важно? Вот пример дерева в посте выше.
Допустим, это список финансовых счетов. Один поток переносит 10 рублей из ноды1 в ноду2.
В ноде1 CAS успешно завершился, а в ноде2 показал, что было изменение. Всё, приехали — данные неконсистентны.
maaGames
03.01.2017 06:52Никогда не рассматривал этот метод в контексте потоковой безопасности. Для меня его основное преимущество в сохранении инварианта, если «что-то пошло не так» и нужно сохранить исходное состояние объекта. Для многопоточности lock как-то безопаснее в общем случае, согласен.
GennPen
02.01.2017 18:56Можно использовать в классе что-то типа такого:
public int VariableWrite { get; set; } public int VariableReadOnly => VariableWrite;
… чтобы ясно было видно с каким доступом обращаешься к переменной.
И странно что у вас в примере Frequency скопирована, а Amplitude нет.aikixd
02.01.2017 19:23И странно что у вас в примере Frequency скопирована, а Amplitude нет.
В этом то и есть проблема. Просто необратил внимание.
Ваш вариант будет работать быстрее. Я только бы вариант для чтения назвал без суффикса, что-бы он привлекательней выглядел. Смущает только наличие
this.
gsaw
02.01.2017 19:03+1Ну еще обычно к имени аттрибута класса добавляют к примеру «m» спереди. Многим кажется лишним, но мне часто помогает определиться, особенно если наворотят метод на пару страниц.
aikixd
02.01.2017 19:26Я раньше тоже не любил приставок, но думаю что сейчас начну добавлять подчеркивание для локальных переменных, что бы не портить внешний вид класса.
GennPen
02.01.2017 19:45У вас в примере итак уже нормально именуются переменные: локальные — с маленькой буквы, класса — с большой, а с подчеркивания обычно private именуются.
aikixd
02.01.2017 20:00+1Я просто думаю что все свойства класса я могу увидеть в автодополнении если напишу
this., а все локальные через подчеркивание. И подчеркивание лучше видно, чем m. И с областью видимости согласуется: У пабликов — большая буква, у приватов — маленькая, у локальных — никакая.
vadim_ig
02.01.2017 23:59Вот как раз хотел написать — к чему усложнения с обязательным this, если можно обойтись правилами именования аттрибутов: наглядно, легко контролировать и очень непросто спутать с той же локальной переменной.
qw1
02.01.2017 19:39Чтобы реализовать проверку правила №2 средствами компилятора, функции могут быть статическими. Невозможно будет получить доступ к полям класса. Скоро c# 7 в этом поможет, позволив писать довольно чисто:
public class SomeClass { int A; int B; private static (int,int) ComputeState(int a, int b) { return (a+b, a-b); } public void ChangeState() { (this.A, this.B) = ComputeState(this.A, this.B); } }
Ещё обещают локальные ф-ции, если бы они тоже понимали static…
impwx
02.01.2017 19:59+3То, что вы пытались изобрести, называется «чистые методы». В C# есть способ реализации куда лучше: делайте как можно больше методов статическими. Тогда в них гарантию неизменяемости объекта вам даст сам компилятор, и тестировать эти методы станет проще. В качестве точек композиции, в которых состояние меняется, останутся немногочисленные экземплярные методы и конструктор.
sashagil
03.01.2017 00:20Вариант: каждый тип — struct (чтобы успокоить себя насчёт производительности) со всеми полями readonly и удобным набором конструкторов. Все методы — чистые (можно для единообразия делать их все extension-методами, пополняя набор методов без вмешательства в определение struct-«ядра» библиотеки; дополнительный плюс такого правила — обязательное именование this параметра в реализации каждого такого extension-метода, как хочет автор этого поста); те из них, которые «мутаторы», возвращают новую («преобразованную») копию struct this-параметра (первого параметра в реализации extension-метода). Бонус: при такой дисциплине цепочечный синтаксис обеспечен для каждого типа «из коробки»: myS = myS.Transform1().Transform2().Transform3();. Лишнее копирование полей this-параметра в локальные переменные не требуется, дисциплина обеспечивает иммьютабельность автоматически. Правила для поддержания этой дисциплины при статической проверке вроде бы должны быть несложными.
vintage
03.01.2017 08:45+2Поставить поле не с той стороны выражения слишком легко.
Рандом драйвен девелопмент — расставляем переменные в случайном порядке, авось заработает?
У нас получается три абзаца. В первом мы объявляем все что нам понадобится, во втором делаем полезную работу, в третьем сохраняем состояние
Вы предлагаете безусловно получать все данные, которые могут потребоваться методу, даже если они фактически не потребуются из-за условия в середине "полезной работы"?
Отсутствие третьего абзаца делает метод чистой функцией,
Не делает, она остаётся недетерминированной.
а значит его можно запускать параллельно.
Нельзя, так как операция чтения всего состояния и записи его обратно не являются атомарными.
aikixd
03.01.2017 11:47+2Рандом драйвен девелопмент
Это было бы смешно, если бы не было так грустно.
Вы предлагаете безусловно получать все данные, которые могут потребоваться методу, даже если они фактически не потребуются из-за условия в середине "полезной работы"?
Это и так происходит:
private int name(int a, int b, int c) { if (b == 0) return a / c; return a / b; }
Если будет тормозить то можно развернуть. Но нужно замерять. В некоторых случаях копировать в стек может быть бестрее, даже если половина данных не будет задействована.
Не делает, она остаётся недетерминированной.
Если все методы внутри нынешнего метода и далее по цепочке написаны правильно, то все изменения состояния должны быть в 3 абзаце. Если я что-то упустил, укажите.
Нельзя, так как операция чтения всего состояния и записи его обратно не являются атомарными.
В этом методе нет записи состояния.
vintage
03.01.2017 23:20Это и так происходит:
А так не происходит:
private int name() { if( this.b == 0 ) return this.a / this.c; return this.a / this.b; }
И так не происходит:
private int name( lazy int a, lazy int b, lazy int c ) { if (b == 0) return a / c; return a / b; }
В некоторых случаях копировать в стек может быть бестрее, даже если половина данных не будет задействована.
Копирование в стек ничего не даст. Современные процессоры/компиляторы сами умеют предзагружать данные в кэш.
Если я что-то упустил, укажите.
Вы упустили собственно детерминированность — зависимость результата выполнения функций исключительно от значений параметров её вызова. Говоря проще, чистая функция не только не изменяет стороннее состояние, но и не читает из изменяемого состояния.
В этом методе нет записи состояния.
Зато есть неатомарное чтение. Если вы запустите несколько таких функций в параллель, а тем временем начнёте менять состояние, то эти функции прочитают ерунду.
aikixd
04.01.2017 15:01А так не происходит:
Да, я это записал в минусах, что может работать медленней. Для такого простого примера, скорее всего даже даже ветвления не будет и процессор все равно подгрузит оба значения. Для более сложных примеров нужно смотреть отдельно.
И так не происходит:
Происходит. Вы все равно передаете указатель на ленивый объект. Lazy имеет смысл если получение объекта дорогое, но если в нем спрятано число или объект уже загруженый в память, то в нем нет смысла.
Копирование в стек ничего не даст. Современные процессоры/компиляторы сами умеют предзагружать данные в кэш.
Процессору нужно сильно помогать, что бы он подгружал нужные данные. Если ваша проблема чувствительна к data locality, то врядли вы будете использовать абстракции. Там все будет прибито гвоздями, отполировано и помечено, чтоб никто не трогал.
Вы упустили собственно детерминированность — зависимость результата выполнения функций исключительно от значений параметров её вызова. Говоря проще, чистая функция не только не изменяет стороннее состояние, но и не читает из изменяемого состояния.
Все так. Но мы в ООП. Писать чистый функциональный код на шарпе будет сложно и некрасиво. В рамках вопроса, можно опустить эту деталь. Если во время выполнения метода, не будете менять состояние объекта из другого места, то проблем не будет.
Зато есть неатомарное чтение. Если вы запустите несколько таких функций в параллель, а тем временем начнёте менять состояние, то эти функции прочитают ерунду.
Конечно. Все равно нужно лочить объект и тд. Но сам метод не напортачит, если запустить его параллельно.
vintage
04.01.2017 17:25Происходит. Вы все равно передаете указатель на ленивый объект. Lazy имеет смысл если получение объекта дорогое, но если в нем спрятано число или объект уже загруженый в память, то в нем нет смысла.
Нет, там передаётся функция получения объекта, которая может быть заинлайнена компилятором.
Там все будет прибито гвоздями, отполировано и помечено, чтоб никто не трогал.
У прибитых гвоздями алгоритмов очень плохо с переносимостью.
В рамках вопроса, можно опустить эту деталь.
Опускать детали вы, конечно, можете, но не надо называть чистыми грязные функции. Ни к чему хорошему это не приведёт.
Но сам метод не напортачит, если запустить его параллельно.
Он выдаст неправильный результат. Оно вам надо?
symbix
Есть языки, в которых this обязателен.
Что касается случайного изменения — думаю, достаточно добавить в юнит-тесты проверку на неизменность внутреннего состояния после вызова метода.
aikixd
Юнит-тесты проверят правильность выполнения а не неправильность. Метод может переписывать состояние только на части домена параметров. И для того что-бы написать тест, который вызовет это поведение, вам нужно будет изучить код метода.
symbix
О, когда часть состояния мутабельная, а часть — нет, это создает огромную путаницу само по себе, особенно при отсутствии языковой поддержки иммутабельности.
Лучше выделить immutable value objects.
aikixd
Я имел ввиду, что метод может изменять состояние только при определенных входных параметрах. Скажем если метод принимает два инта и если они оба простые числа, то тогда что-то ломается. Такие вещи тяжело проверить юнитами. Даже контракты, которые предназначены именно для поиска кривого кода, могут не словить это до продакшена. Я хочу предотвратить проблему еще до компиляции.
А то что вы описали — правда. Но иногда неизменность должна поддерживаться во временных рамках. Скажем как в моем коде, где я ошибся. Нет проблемы если амплитуда поменяется до или после, главное не во время. Было бы классно, если бы можно было помечать методы, которые не имеют права менять состояние. А еще лучше помечать те, которые имеют.