Комментарии 32
Вот в общем-то поэтому из Rust исключили наследование и понятие класса как таковое, а оставили только struct и trait. Программисты на С++/C#/Java называют это "интерфейс".
Очень информативно, спасибо! Преподаю unity на начальном уровне для старта и уже хочется самому глубже залезать в архитектуру, очень хорошо раскрываете суть)
Только ссылки надо поправить, кажется, они ведут не в тг, а на ютуб юнити
Да, спасибо. Исправил. Видимо с утра запутался в своих ссылках и с поста забрал. Но видос там был тоже интересный :)
Неа. Обе ссылки на ютуб...
Это видимо кеш. У меня уже ок, ща протестил. Но если у кого вдруг ещё так лагает, то вот отдельно https://t.me/+ZypFCP_rxic4NTEy
Интерфейс в c# изначально задумывался, как контракт между классом и интерфейсом. Каждый класс, заключивший контракт, обязан предоставить реализацию, а метод - способ этой реализации управления данными. По этой причине, например, нельзя было до C# 11 иметь поля в нём от слова совсем, иначе это нарушало инкапсуляцию. Компилятор ещё тот параноик.
Итерацию с использованием ECS ждать?
Что если сделать AttackData структурой, чтобы избежать аллокаций в AttackManager, а значения полей получать через геттер в интерфейсе?
Да, норм улучшение. Но есть два нюанса.
Первое, что такие маленькие объекты пойдут в малую кучу, поэтому это не так критично. Юнити вроде как исправили то, что малой кучи в юнити не было.
А во вторых в шарпе нет наследования структур. Поэтому возникает вопрос «как расширять число полей». Композиция интерфейсов и адаптер. Ну может быть, но не уверен, что это будет сильно лучше.
Плюс основной вопрос. А зачем это оптимизировать? AOEAttack — это структура. Так как её аллоцировать мы будем часто. А AttackData чаще всего на запуске приложения и всё. Так как это данные оружия. Или при выпадении предмета забирая данные из базы. Или в момент загрузки юзера, чтобы посмотреть инвентарь. То есть вряд ли тут аллокации будут когда-то создавать фризы.
И жертвовать ради этого удобным расширением числа полей — не уверен, что стоит.
Мне кажется, лучше делать метод Attack не в классе атаки, а в WorldManager. В реальности есть физические законы взаимодействия материи, оружие их использует, чтобы нанести урон. В игре нам не нужно детализировать WorldManager до такой степени, поэтому мы можем задать там более высокоуровневые законы взаимодействия (огонь, магию и т.д.), но это все равно часть WorldManager, он должен о них знать. Так не будет глобальной переменной в классе атаки, и WorldManager может ее модифицировать в зависимости от расстояния, или создавать новые атаки, например при выстреле в взрывающийся объект.
Не очень у вас ООП получилось.
Сначала:
_weapon = new Weapon()
{
AttackData = new AOEAttackData()
{
AttackType = AttackType.AOE,
}
// остальное пропущено
}Вы два раза указываете тип атаки - в new и в enum-поле. Нарушена инкапсуляция.
Далее вы делаете:
var attack = AttackManager.GetAttack(transform.position, _weapon.AttackData);
public static IAttack GetAttack(Vector3 position, AttackData data)
{
switch (data.AttackType)
{
case AttackType.AOE:
return data is AOEAttackData aoeData ? new AOEAttack(position, aoeData) : null;
case AttackType.Splash:
return data is SplashAttackData splashData ? new SplashAttack(position, splashData) : null;
}
}Это вообще жесть, вы проверяете и enum-свойство и тип AttackData. Хотя достаточно одной проверки, но из-за нарушения инкапсуляции выше, получилось две. Да и в целом нарушает OCP (O в SOLID)
По хорошему надо было сделать полиморфный метод GetAttack в AttackDataи писать так:
var attack = _weapon.AttackData.GetAttack(transform.position)или еще лучше, чтобы не нарушать Закон Деметры (D в SOLID):
var attack = _weapon.GetAttack(transform.position)Внутри метода Attackобращение к WorldManager выглядит крайне плохо, хреново тестируется и нарушает SRP и ISP (SOLID). По хорошему стоило бы написать так:
private void Update()
{
if (Input.GetKeyDown(KeyCode.A))
{
var attack = _weapon.AttackData.GetAttack();
// _worldManager нужного типа подсовывается в Player через DI
attack.Attack(_worldManager, transform.position);
}
}А в идеале поделить один Attack на два метода - получение списка целей и нанесение урона.
Выглядело бы как-то так:
private void Update()
{
if (Input.GetKeyDown(KeyCode.A))
{
var attack = _weapon.AttackData.GetAttack();
// _worldManager нужного типа подсовывается в Player через DI
var damagables = _worldManager.GetDamagable(transform.position, attack);
foreach(var damagable in damagables)
{
damagable.TakeDamage(_weapon.AttackData.Damage);
}
}
}
// Делаеим Double Dispatch
public class WorldManager: IWorldManager
{
IEnumerator<IDamagable> GetDamagable(Vector3 position, IAttack attack) => attack.GetDamagable(this, transform.position)
}
// Кстати struct вам только помешает, ибо вы обращаетесь к нему через интерфейс
public struct AOEAttack : IAttack
{
private AOEAttackData _attackData;
public AOEAttack(AOEAttackData data)
{
_attackData = data;
}
public void Attack(IWorldManager worldManager, Vector3 position) => WorldManager.GetDamagable(position, _attackData.Area);
}Пока написал понял что деление наXXXAttack иXXXAttackData не нужно. В целом у вас только одна точка где вам нужен полиморфизм - GetDamagable. Для полиморфизма в одном месте вам не надо городить две параллельные иерархии. Параллельные иерархии это вообще code smell.
Итак. Спасибо за ответ, всегда интересно послушать альтернативное мнение, но пройдёмся :)
Инкапсуляция — это в контексте задачи про композицию не важно.
Двойные типы. Да согласен. Это лишнее.
Двойная проверка в фабрике. Да тут и вся эта фабрика лишняя. Она мне нужна была для примера связывания в композиции.
А вот теперь про GetAttack в AttackData и почему нет. AttackData это модель, по хорошему она не должна определять поведение. Потому что много что может не зависеть от позиции и т.п. Хотя у вас она зависит, а потом резко не зависит.
Плюс это всё выглядит красиво до реальной реализации. Так как дальше появляется ворлд менеджер, который принимает на вход атаку. И мы получаем проблему супер жирного контроллера. Так как менеджер мира знает, как собрать данные о целях для конкретной атаки. И теперь у нас игрок собирает цели и атакует их. Это не совсем верная логика системы. И у нас SRP нарушает менеджер мира.
В моей же системе менеджер не знает ничего про атаку. Он забирает просто данные по площади в позиции не зная о таком понятии, как атака.
И логика этого проста. Атака сама реализует логику своей обработки, так как это её ответственность.
Внутри метода
Attackобращение кWorldManagerвыглядит крайне плохо, хреново тестируется и нарушает SRP и ISP (SOLID).
То есть это чушь. Так как вызов WorldManager в атаке — не нарушает сингл респонсибилити. Так как наш запрос не меняет никакого состояния волрд менеджера. У нас есть стейт, мы из него запрашиваем данные для реализации нашей функциональности. Вот и всё. Там не усложняется никак тестирование. Так как волд менеджеру вообще должно плевать кто его вызывает. Он не в курсе. Если про тестирование атаки, чтобы подсунуть в неё мок менеджера. А для чего и зачем? В миниатюрный класс в 10 строк пихать юнит тест. Без мок мира целиком (то есть не интеграционный). А для чего?
Передавать его в качестве параметра метода тоже нет никакого смысла. Для сингл таргет цели мы передадим в конструктор нашей атаки конкретную цель допустим. И это будет в разы удобнее.
Последний блок кода в целом странный, но мне кажется вы просто запутались. Так как там что-то странное в AOEAttack.
Пока написал понял что деление на
XXXAttackиXXXAttackDataне нужно. В целом у вас только одна точка где вам нужен полиморфизм -GetDamagable.Для полиморфизма в одном месте вам не надо городить две параллельные иерархии. Параллельные иерархии это вообще code smell.
Оно не обязательно было бы без оптимизаций и специфики. И вопрос подхода к архитектуре. Структура тут нужна для оптимизации. Так как атаки — это часто аллоцируемые команды. Которые поэтому нехорошо делать в виде классов. А данные (а точнее поля) расширять без классов неудобно. Это по сути про отличие объектов поведения, и объектов данных.
Можно было бы определять в XXXAttackData без XXXAttack без этой оптимизации. Если бы не ещё одно но. Разные поведения могут опираться на общие данные. Это в конкретной реализации получаются параллельные иерархии. А в реальной игре иерархия данных вполне может отличаться от иерархии объектов поведения. Опять-таки для удобства.
Инкапсуляция — это в контексте задачи про композицию не важно.
Слишком громкое заявление. Если вы нарушаете инкапсуляцию, то радо или поздно столкнетесь с проблемами. Это как гигиена. Можно раз не почистить зубы, но если так делать постоянно, то зубов у вас не останется довольно быстро.
Так как вызов WorldManager в атаке — не нарушает сингл респонсибилити. Так как наш запрос не меняет никакого состояния волрд менеджера.
Если про тестирование атаки, чтобы подсунуть в неё мок менеджера. А для чего и зачем? В минитюрный класс в 10 строк пихать юнит тест. Без мок мира целиком (то есть не интеграционный). А для чего?
Взаимоисключающие параграфы.
Дело не в том зачем писать тест. Дело в том что если тест написать легко, то как минимум принципы SOLID не нарушены. Если нет, то скорее всего нарушены.
В целом нет никаких формальных доказательств что SOLID делает архитектуру лучше, но эмпирические очень даже есть.
Из академического интереса напишите тест для реализаций IAttack, увидите кто чего нарушает.
Структура тут нужна для оптимизации. Так как атаки — это часто аллоцируемые команды.
Получая ссылку на интерфейс структуры вы делаете боксинг структуры. В курсе? Боксинг это аллокация и копирование данных из стека в кучу с последующем удалением сборщиком мусора.
Вы занимаетесь преждевременной оптимизацией, причем неудачно.
И вопрос подхода к архитектуре.
К сожалению ваш подход работает хуже, чем мог бы.
----------------------------------------
Это об было архитектуре вообще. А теперь частности:
AttackData это модель, по хорошему она не должна определять поведение.
Честно говоря не пойму кто кому чего должен. Вы сначала придумали классы и объекты, а потом пытаетесь к ним прикрутить какое-то поведение или не прикрутить. Не надо так делать.
Так как дальше появляется волд менеджер, который принимает на вход атаку. И мы получаем проблему супер жирного контроллера.
Метод можно сделать Extension_ом и все будет хорошо. Это я понял уже после того как написал.
public static IEnumerator<IDamagable> GetDamagable(this IWorldManager manager, Vector3 position, IAttack attack) => manager.GetDamagable(this, transform.position)Остальной код не поменяется.
Это не совсем верная логика системы.
Это субъективное заявление. Просто логика мира в вашей голове отличается от логики в других головах. Помните всегда об этом.
И у нас SRP нарушает менеджер мира.
Он и так будет SRP нарушать. Это фасад к куче разных подсистем. По хорошему надо его разделить на эти самые подсистемы и инжектить конкретные подсистемы в конкретные классы. Но так снижается discoverability. Кроме того подсистемы связаны между собой, тогда разделение может нанести вреда больше чем пользы.
Про SRP да согласен тупанул. Там же ответственность не инкапсулирована целиком в класс.
Он и так будет SRP нарушать. Это фасад к куче разных подсистем.
Ну в моей идее не совсем, так как тут плохое название. Но нет, он не будет фассадом, так как допустим в сингл таргете у нас вообще не будет такого менеджера. И если лоцировать, то через конструктор команды.
А так спасибо за дополнения.
Закон Деметры (D в SOLID)
Это что-то новенькое.
Выше я объяснил что не так в вашем коде, а теперь попробую донести ошибки мышления, которые привели к ошибкам в коде:
Атака — это поведение
Нет, нет и еще раз нет. Я не смогу объяснить это лучше, чем сделал Эрик Липперт в своей классической серии статей https://ericlippert.com/2015/04/27/wizards-and-warriors-part-one/ и далее. Когда-нибудь я переведу их для хабра.
Вкратце суть: не надо строить иерархии классов на основе объектов игрового мира (или того, что вы моделируете в своей программе).
Во-первых, нам нужно создать атаку.
...
По сути тут фабрика
Вы хотели применить паттерн (фабричный метод), но никаких оснований для его применения не было, потому что тип вам и так известен в этой точке программы, вы его явно задали выше. Фабричный метод нужен только тогда, когда вы НЕ знаете в данной точке программы какой тип вам нужен.
Почему атаки — это команды?
Потому что любая игра это цикл: анализ текущего состояния мира + анализ ввода -> генерация команд -> изменение состояния мира.
С этого надо было начинать.
Ключевая ошибка - проектирование снизу вверх. Вы сначала придумываете объекты игрового мира, поведение, а потом пытаетесь это все засунуть в правильную архитектуру.
Просто пойдите другим путем. Начните с простого цикла, как я описал выше. А потом декомпозируйте функции, заменяйте большие неуклюжие if и case на полиморфные методы, применяйте паттерны там где они уместны и у вас родится идеальная иерархия классов.
Нет, нет и еще раз нет. Я не смогу объяснить это лучше, чем сделал Эрик Липперт в своей классической серии статей https://ericlippert.com/2015/04/27/wizards-and-warriors-part-one/ и далее. Когда-нибудь я переведу их для хабра.
Стоп-стоп-стоп. Эрик Липперт говорит не про атаку. И то что это поведение никаким образом не относится к его заметке. И эту проблему как раз и решает композиция. То что описано на пол статьи, почему не надо наследовать оружие и т.п. Мышление идёт нет не от объектов игрового мира, а от процессов. Абстрактное проектирование вундерфавли я в целом не приветствую. Но это другой вопрос. Просто эта заметка вообще не про то.
Идеальной архитектуры не существуют. Давайте поговорим теперь в продуктово-бизнесовом ключе. Какие недостатки у этой архитектуры объективные? Риски и проблемы? Долгосрочные. В контексте реальной реализации. Я просто поэтому не люблю все архитектурные споры, особенно с адептами солида, так как время тратящееся на соблюдение всех мантр SOLID в среднем проекте просто не окупается.
Но изначально, я не строил идеальную архитектуру. Я просто составил задачу на композицию и решил её. Лучше, чем была изначальная схема. Вот и всё.
Важно наверное сделать ремарку. А то может мой ответ может звучать как-то не так. И кто-то обижается на "адептов солида". Я ни в коем случае не принижаю вашу экспертизу. Я думаю, что вы так же, как и я сделали уже больше 100 проектов за карьеру.
Я просто не люблю разбор "что не так по солиду". Так как мне столько историй рассказывают про синглтоны те же. Что это смертный грех. Но за 100+ проектов (и три крупных с продакшенами по несколько лет и большими командами) я их там видел. И они не ломаются каждые 5 минут, как предсказывали "свидетели Солид". Я просто не люблю аргументацию "нарушается такой-то принцип солид".
Аргумент с тестированием скажем хороший, но в реальном контексте системы несущественный. Так как такой класс нет никакого смысла покрывать юнит тестом. Если очень хочется, можно выкинуть в конструктор присвоение менеджера. Можно вообще не знать о менеджере и передавать массив юнитов. Там можно так глубоко уходить в "а можно вот так, почему атака знает о каком-то там менеджере, если ей нужны юниты", "а если у нас специфика юнитов, то зачем нам разные типы атаки, мы же всегда можем передавать массив юнитов". И крутить этот кубик-рубика в любую сторону. Находя аргументацию любого из подходов.
Я же люблю говорить о бизнесовых и продуктовых недостатках. В сайд эффектах таких решений. Например дублирование иерархии. Нам придётся писать код в трёх местах, чтобы добавить фичу. Это плохо. Это лишний контракт. Его можно избежать. Но когда это 40 строк кода — это не крит. И тут не надо уходить в крайности главное, хотя велик соблазн. И я когда-то так делал в таких спорах. "Так про всё можно сказать в таком случае". Нет нельзя. Так как есть логика системы и понимание из опыта, что возможно, а что нет.
Раз вы за бизнес-подход, то вы наверное понимаете, что есть формальные (независимые от мнения) метрики: количество строк кода, количество классов, цикломатическая сложность, связность классов. Чем выше эти метрики, тем дороже стоит написание и поддержка кода (последующее внесение изменений в код).
Если эмпирические данные, что соблюдение SOLID в целом снижает эти метрики. Это не вопрос нравится SOLID или нет.
Понятно что в коде на 40 строк все это не нужно. Но вы этот код пишите для того, чтобы нам показать как надо, в том числе когда кода станет не 40 строк, а 40к.
Да, но там не будет 40к. Типов атаки ну 20 придумать сложно даже. Просто если мы начнём это обсуждать и дискутировать, то я уверен, что мы дойдём, что атака вообще должна быть одна. Так как она принимает таргеты и всё. И это будет лучше. Какая разница как они определяются атаке? А определение таргетов должно быть в другом месте. И может для сериализации удобнее будет енам, который идёт куда-то. Тут немного другой контекст наверное.
То есть скажем так. Ваше дополнение ценно. И моя эмоциональная реакция неверная. Минусы задевают самолюбие, как и критика. Особенно исходя из того как хабр ранжирует статьи. И я постепенно учусь это гасить (вот уже 40 статей).
Просто суть статьи для меня немного не в этом. Да и суть задачи. Задача была исправить изначально кривую архитектуру на лучше. Так как формулировать задачи и их решения большой блок работы. И я считаю эту задачу хорошим упражнением.
Как говорится, я как и любой человек не идеален. И то, что статья порождает дискуссии это хорошо.
Но на тему бизнес-метрик, цикломатическая сложность, поддержка кода и т.п. Вот это уже зависит от масштаба разработки и сроков проектировки этого дела. Тут есть нюанс и контекст. Абстрактная архитектура дольше проектируется и дороже. И то что время на проектировку фичи, которая выдержит расширение на 40к строк, при том что потолок тут 1к строк теоретический не факт, что окупит затраты на поддержку. И не факт, что затраты на поддержку будут ниже, чем затраты на проектировку и разработку. И я говорил про кубик-рубика в этом контексте скорее. То есть мы можем потратить месяц на проектировку и рефакторинг к идеалу. А можем написать реализацию за один день и не встретить проблем.
И условно на мой взгляд есть уровень проектировки удовлетворяющий условиям задачи, чтобы не уходить в оверинжиниринг. Так как доводить код до совершенства может быть очень долго, но не выгодно с точки зрения сроков реализации какой-либо фичи. То есть ваше вполне корректное расширение на базе моего решения может быть верным. Но вы потратили время поверх моего времени и так далее.
Да, но там не будет 40к. Типов атаки ну 20 придумать сложно даже.
В программировании есть три числа 0, 1 и N. Все N равнозначны. Если вы показываете пример как надо для N=2, то он должен работать для любого N (по крайней мере в разумных пределах).
Но на тему бизнес-метрик, цикломатическая сложность, поддержка кода и т.п. Вот это уже зависит от масштаба разработки и сроков проектировки этого дела.
Смотрите выше замечание про N. Нам неинтересно как вы решаете задачу для N=2, покажите нам способ решения для любого разумного N, особенно с учетом того, что N будет меняться в процессе разработки и поддержки.
И то что время на проектировку фичи, которая выдержит расширение на 40к строк, при том что потолок тут 1к строк теоретический не факт, что окупит затраты на поддержку.
Еще раз посмотрите на первое замечание про N.
Задача была исправить изначально кривую архитектуру на лучше.
Увы предыдущую архитектуру не видел, но эта статья показывает что у вас еще есть возможность сделать лучше.
Абстрактная архитектура дольше проектируется и дороже
Так не занимайтесь этим. Начните писать код без наследования и полиморфизма вообще. Прям в методе Update пишите.
Если вдруг метод перестанет умещаться на экране редактора кода, то сделайте Extract method.
Если код дублируется, то тоже сделайте Extract method.
Если у вас много функций с одними и теми же параметрами, то перенесите их в отдельный класс.
Потом сделайте Extract Interface и ссылайтесь только на интерфейс.
Когда вам второй раз повстречается
switch(attackType)сделайте интерфейсIAttackи несколько реализаций вместо этогоswitch.
Думаю суть вы поняли.
И условно на мой взгляд есть уровень проектировки удовлетворяющий условиям задачи, чтобы не уходить в оверинжиниринг.
Конечно есть, я выше описал как этого уровня достичь и не промахнуться.
Увы предыдущую архитектуру не видел, но эта статья показывает что у вас еще есть возможность сделать лучше.
Первый скрин в статье.
Так не занимайтесь этим. Начните писать код без наследования и полиморфизма вообще. Прям в методе
Updateпишите.
Это уже уход в крайность и поэтому неинтересно обсуждать.
В программировании есть три числа 0, 1 и N. Все N равнозначны. Если вы показываете пример как надо для N=2, то он должен работать для любого N (по крайней мере в разумных пределах).
Не согласен с аналогией. Так как у нас Nmax определено выше.
Первый скрин в статье.
Диаграмма классов не имеет никакого смысла без примера кода их использования. Именно использование вашей иерархии классов может показать хорошая архитектура или нет.
Это уже уход в крайность и поэтому неинтересно обсуждать.
Это путь познания. Сделайте так один раз и вы будете понимать в ООП гораздо лучше других.
Не согласен с аналогией. Так как у нас Nmax определено выше.
Это у Вас (автора) определено. Нам (читателям) не интересен ваш Nmax. Мой Nmax может оказаться в 1000 раз больше вашего, подойдет ли мне ваш подход?
Видимо вы еще не прочитали до конца весь цикл статей Липперта (их там семь штук). А если успели прочитать за менее чем за 15 минут, то вряд ли успели осознать.
По-моему первая картинка выглядит как результат рефакторинга последней :) Функционал идентичный, но разница в простоте огромная.
Судя по статье - какие варианты расширения предполагает автор? Добавление новых типов оружия. В начальном случае для добавления нового типа оружия нужно добавить... новый наследник Weapon. Звучит просто и логично.
А во втором? Новые наследники IAttack и AttackData, да ещё не забыть добавить кейс в AttackManager.GetAttack
И почему тогда второй вариант гибче первого, если добавлять во втором приходится больше, и в несколько мест? Одна из задач архитектуры - локализовать точки расширяемости кода, и первый вариант справляется с этой задачей лучше.
Вообще архитектура, т.е. добавление слоёв абстракции - это увеличение сложности программы, и за счёт этого должны решаться какие-то проблемы, но в статье не показаны никакие проблемы первого варианта, поэтому все эти трансформации выглядят как оверинженеринг.
а где такой конструктор блок-схем добыть?
Задачка «Вооружимся архитектурой»