Расширение класса внешними методами и переменными

whirlwind · 7 Мар 2020

fixxxer написал(а):
Не знаю насчет Вурдалака, у меня и такие, и такие, в зависимости от. Раз ты уж так привязался к show me the code, ну вот такая фигня на гитхабе валяется.

Я сильно углубляться не буду, но вот смотри по верхам. RouteDispatcher хороший пример демонстрации отсутствия проблемы. Есть у тебя контроллер. Под ним нет никакой инфраструктуры. Для него по сути фабрика нужна что бы по имени класс инстанцировать - все. Где DI тут? Полиморфизм не нужен. С другой стороны, роутер не требует контракта от контроллера. То есть, с этой стороны полиморфизм тоже не нужен. Отсюда следует, что вы там все можете зафайналить и никто не пострадает.

Дальше. Честный тест на dispatch не сделан. Посчитай количество взаимоисключающих ветвей внутри форыча и возведи в квадрат минимум (минимум 2 элемента = 2 прохода), что бы обозначить работу цикла. Что бы тестить жирные циклы нужно тело цикла выносить за контрактный метод (назовем этот контракт хелпером) и отдельно тестировать 2 кейса: когда код идет по циклу, а отдельно тестить один проход по циклу (тест логики обработки элемента). Если обе части покрыты на 100%, значит и в комбинации они работают правильно. Но общий тест без выноса будет очень сложен, что и демонстрирует твой тест. И смотрим опять: DI нет, 100% покрытия нет. Нет DI, нет понимания что это внутренний дизайн и лепить тут implements на класс логики обработки элемента бессмыслено. Этот хелпер класс должен быть package visible, а юзер (диспатчер в данном случае) будет предусматривать два конструктора: package visible с возможностью инжектнуть мокнутый хелпер и дефолтный конструктор, который инстанцирует реальный хелпер.

Ну и так далее по тексту.

fixxxer · 7 Мар 2020

Что ты понимаешь под DI? Контейнера конечно нет, делать контейнер на такую микробиблиотеку бессмысленно. Есть фабрика из одного метода, которая просто-напросто заменяется на другую (или вообще на Laravel-овский DIC), если надо подсунуть другие реализации. А DIP соблюдается.

Что касается RouteDispatcher::dispatch - это ты хорошо подметил самое архитектурно проблемное место. В первой версии цикломатическая сложность там была куда ниже, и такой дизайн выглядел более уместно (даже идея подсунуть туда другой диспетчер не выглядела так глупо - идея была в том, что кому-то может понадобиться вообще другой вид диспетчеризации, не по принципу controller-method), а потом мне было уже лень заниматься декомпозицией. Тесты из-за этого получились переусложненными, но они, если приглядеться, неплохо покрывают все это разнообразие. По крайней мере, все серьезные проблемы в ходе того дописывания они успешно ловили.

fixxxer · 7 Мар 2020

whirlwind написал(а):
юзер (диспатчер в данном случае) будет предусматривать два конструктора: package visible с возможностью инжектнуть мокнутый хелпер и дефолтный конструктор, который инстанцирует реальный хелпер

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

Ну и какой package visible в php?

Только с анонимными классами если извращаться, но это как-то совсем уж.

whirlwind · 7 Мар 2020

fixxxer написал(а):
Это становится частью контракта

Нет! Контракт - это паблик интерфейс. Например inner classes в Java как раз для таких целей - дать поле для маневра при сохранении инкапсуляции. Ну сделай protected ctor или фабричный метод. Это уже детали. Было бы желание.

DI это не обязательно IoC контейнер, DI это когда ты пушиш зависимости снаружи вовнутрь, а не пуллиш их изнутри (как в случае SL). У IoC и SL свои тонкости, но то отдельная большая тема. И там и там ключевой момент - полиморфизм. Если один объект не может прикинуться другим, то LSP не работает. А интерфейсы и абстрактные классы - это вообще чисто локальная языковая фишка.

fixxxer · 7 Мар 2020

whirlwind написал(а):
DI это не обязательно IoC контейнер, DI это когда ты пушиш зависимости снаружи вовнутрь

Я так и делаю. Или ты до FormRequestFactory докопался? Да забей на него, это вообще для одного чувака с гитхаба сделал за полчаса, это банальная обертка вокруг Laravel-овской инфраструктуры, считай, что inner class. Можно было бы, конечно, вынести в аргумент конструктора или сделать отдельный named, ну, да, но там это не роляет вообще. Она там сама по себе тот еще костыль.

whirlwind написал(а):
Например inner classes в Java

Если бы у бабушки была Java...

whirlwind написал(а):
Ну сделай protected или фабричный метод. Это уже детали.

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

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

whirlwind · 7 Мар 2020

Мы все всегда говорим о том кто чего не заслуживает. 100% честный блэкбокс тест на void метод с одним int аргументом должен быть вызван для 2^32 значений. Но никто так не делает, потому что оно того не стоит. Тоже самое с файналом. Польза от него крайне сомнительна, а вред огромен. Очешуенная, популярная и всем известная либа gson охрененна всем, кроме того, что с тестами придется изрядно заморочиться. Ибо какой-то умник сделал final на фасад который не имплемент. Примеры вреда я могу приводить бесконечно ))

fixxxer · 7 Мар 2020

Ну где ты у меня там final без implement нашел-то, а?

Проблема RouteDispatcher (которую ты хорошо отметил) там вовсе не в final, а в отсутствии декомпозиции. Убрать final и поменять private на два protected-а - это не решение, это костыль чисто для тестов. Если кто-то решит от него унаследоваться, это будет полным фаталити - придется обеспечивать BC для всей этой внутрянки.

whirlwind · 7 Мар 2020

Я не про тебя, а в общем. В идеале про final я не уверен, а в среднем - он вреден. Вы же утверждали что оно панацея. Когда покажете тесты уровня 2^32 на 1 int аргумент, тогда соглашусь ))) А то моду взяли, как тесты писать так можно пофилонить, а как стереотипы распространять - так сразу прям идеальный у вас код )))

fixxxer · 7 Мар 2020

whirlwind написал(а):
так сразу прям идеальный у вас код

Эээ, вот не надо! Я сразу предупредил:

fixxxer написал(а):
Там, конечно, все далеко не идеально

fixxxer · 7 Мар 2020

И, да, про inner classes - в том же Typescript я так часто делаю. А в PHP при соблюдении DIP так или иначе вся "внутрянка" становится публичной. Тут либо излишняя декомпозиция, чтобы предоставлять стабильное API, либо вот так. Что поделать, PHP говно))

whirlwind · 7 Мар 2020

Короче, как обычно, спор выиграл Гриша, который пару страниц назад дал правильный ответ - "принимать под присмотром врача" или как то так

fixxxer · 7 Мар 2020

whirlwind написал(а):
Когда покажете тесты уровня 2^32 на 1 int аргумент

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

Тут тоже надо с умом подходить, а не молиться богу покрытия.

Yoskaldyr · 17 Мар 2020

Пока вирусная паника под шумок этот трешовый rfc кажется примут

PHP: rfc:write_once_properties

wiki.php.net

write-once пропертис которые будут называться readonly - просто фейспалм

fixxxer · 17 Мар 2020

Mixed feelings.

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

С другой стороны, возможность присвоить один раз даже снаружи - это какой-то facepalm.

Yoskaldyr · 17 Мар 2020

fixxxer написал(а):
С другой стороны, возможность присвоить один раз даже снаружи - это какой-то facepalm.

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

WMix · 17 Мар 2020

Yoskaldyr написал(а):
в больше половины случаев

написано же, все ради lazy load

fixxxer · 17 Мар 2020

это очень странный lazy load, для которого надо делать unset в конструкторе (!) и извращаться с __get. Уж проще геттером.

WMix · 17 Мар 2020

fixxxer написал(а):
для которого надо делать unset в конструкторе (!)

о чем ты? зачем unset?

PHP:
Another restriction of “write-once” properties is that they can't have a default value. Thus, the following syntax is forbidden:

PHP:

class Foo { <keyword> public int $a = 0; }

я так понял подразумевается это

PHP:

$obj = new Foo();
$obj->a = 42;

Yoskaldyr · 17 Мар 2020

Another restriction of “write-once” properties is that they can't have a default value. Thus, the following syntax is forbidden:

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

fixxxer · 17 Мар 2020

WMix написал(а):
о чем ты? зачем unset?

Add support for "write-once" properties by kocsismate · Pull Request #5186 · php/php-src

Currently, the implementation makes use of the final keyword, although the actual name is to be decided. RFC: https://wiki.php.net/rfc/write_once_properties

github.com

Расширение класса внешними методами и переменными

TDD infected, paranoid

К.О.

К.О.

TDD infected, paranoid

К.О.

TDD infected, paranoid

К.О.

TDD infected, paranoid

К.О.

К.О.

TDD infected, paranoid

К.О.

"Спамер"

К.О.

"Спамер"

герр M:)ller

К.О.

герр M:)ller

"Спамер"

К.О.