Проблема в том, что нигде в статье не были описаны требования к инвентарю, кроме картинки, чтобы говорить о чем-то конкретном. В статье есть очень удачное предложение, которое характеризует большинство моих пунктов, что до ввода MonoBehaviour можно было играть в консоли. Они описывают тесную связь логики и представления с примесью статических классов. Никакие интерфейсы, абстракции и события в отделении логики от представления не помогут в этом случае, если их использовать таким образом. Код же необходимо писать таким образом, чтобы он был понятен другому разработчику, а не потому что и так все работает. Большую часть времени код читается, а не пишется.
В User нарушен SRP, потому что он управляет жизненным циклом инвентаря, управляет сохранением и загрузкой состояния, прибит гвоздями к конкретной реализации сериализации. Атрибут Serializable не требует наличие атрибута JsonProperty для сериализации классом JsonUtility, а сериализация классом JsonConvert не требует атрибут JsonProperty для сериализации всех свойств. Т.е. добавляется еще головная боль с тем, а всё ли можно засериализовать, и с тем, что нужно сериализовать, а что нет.
Касательно Null Object, речь про то, что у объекта два состояния: объект есть и объекта нет. Поведение с иконкой вылезло за границы объекта. Я знаю, что в Unity такое обычно решается через шаблон состояние.
Instantiate и зарабатывание миллионов долларов - связанные вещи. Мне так надоело, когда в том же Steam, появляется очередная игра усеянная багами, что иногда даже запустить ее невозможно. Конкретно здесь речь, когда условный тетрис (очень простая игра), отжирает огромное количество ресурсов или падает при странных условиях. Некоторые разработчики даже не удосуживаются банально проверять локаль системы, из-за чего падает DateTime, зато продают игру за 30 долларов. ААА игры тоже страдают от слабого уровня программистов, например Wasteland 3 долгое время грузил уровни по несколько минут, сбрасывал прогресс при загрузке сохранения. Главное верить, что озлобленные потребители не вернут деньги и не перестанут покупать будущие игры.
ScriptableObject обычно используется именно так, как я описал. Бывает даже в него домешивают поведение, никто же не запрещает. Был даже один довольно популярный доклад, который предлагал все на них делать. Такая веселая сущность, что даже сбрасывает все свои свойства при небольшом рефакторинге.
У меня статья вызывает больше недопонимания. Оговорюсь сразу, я очень далек от Unity.
Не существует никакого "идеального" инвентаря. Любая разработка начинается с определения набора требований. Как я должен понять требования к инвентарю по описанию эффекта Даннинга-Крюгера и некой непонятой картинке инвентаря? Что означает каждая секция инвентаря? Почему у ячейки инвентаря нет количества предметов?
Почему абстрактный класс BaseItem без ключевого слово abstract? Зачем вообще строить какую-то невнятную иерархию наследования без обозначения, кто его наследники? Какая вообще проблема решается через наследование? Почему просто не создать структуру только для чтения под названием InventoryCell (BaseItem - слишком абстрактное название)?
Почему ID у BaseItem задан как long? Я вижу сходу две явные реализации ID: как Enum (когда заранее известны все возможные предметы) и как еще одна структура только для чтения (когда все виды предметов загружаются динамически из ресурсов, когда надо валидировать ID и чтобы не дать возможность ID присвоить, скажем, CellID).
Почему количество ячеек в инвентаря определяется динамически, если на картинке нет полос прокруток?
Почему у Inventory ровно один интерфейс с очень странным названием IInventoryActions? Почему не просто IInventory? Зачем этот интерфейс вообще нужен, если в классе User он не используется? Почему IInventory не наследуется хотя бы от IEnumerable? Почему не используется индексатор для доступа к ячейке инвентаря? Зачем Add и Remove возвращают bool? Я должен поверить на слово? Это прямое нарушение YAGNI и принципа единственной ответственности из SOLID.
UIInventoryWindow. С какой целью абсолютно все поля во всех наследниках MonoBehaviour помечены атрибутом SerializeField, чтобы было, вдруг пригодится? Почему вообще вызывается Instantiate для ячейки, неужели нельзя обойтись без этого? Почему инвентарь не попадает в UIInventoryWindow через инверсию зависимости?
Почему UIInventoryCell содержит OnClickCell? Где вообще было сказано, что по ячейкам кликают мышкой? Почему OnClickCell не часть BaseItem? Почему у события неправильное название? OnCellClicked - это название для обработчика события, а не для события. Зачем вообще нужно это событие, какая проблема решается? У такой реализации событий очень много проблем, что проще забыть вообще про ключевое слово event. Одна из проблем прямо наглядно видна в методе OnPointerClick с ненужной проверкой на null.
Почему у класса User нет интерфейса? Почему приватное поле названо с большой буквы? Почему User знает как создавать инвентарь и самого себя? Почему не используется современный C# (или вовсе избавиться от приватного поля):
public static User Current
{
get => _current ?? new User();
}
И что с того, что в играх на одного человека всего один игрок? Как этот код поддерживать? Как написать тесты на этот код? Про существование шаблона одиночка лучше вообще забыть.
ID чего принимает GetData в интерфейсе IStorageActions? Зачем здесь вообще обобщение используется?
В классе ItemVisualData (плохое название, класс описывает информацию о предмете) можно повесить атрибут SerializeField на свойство. Зачем вообще нужен этот атрибут? Для сериализации он не нужен. Почему VisualName - это строка, а не ID ресурса локализации? Статическое и только для чтения - это разные слова с разным смыслом, т.е. "статическое хранилище" - вообще не означает "только для чтения".
Зачем в классе ItemsVisualDataStorage метод Load? Не проще ли перенести содержимое метода в конструктор? И не надо в словаре вызывать метод ContainsKey, достаточно использовать индексатор.
ScriptableObject - это очень ужасное дизайнерское решение разработчиков Unity, что-то сопоставимое с шаблоном одиночка. Не надо в коде использовать сокращения, такие как SO. Почему эти данные не берутся из ресурсов?
Для тестов надо писать тесты, а не портить класс User.
Storage и Inventory. Раз приходится пояснять, почему это не одно и тоже, значит в коде что-то не так. У меня прослеживается логика, что Inventory хранит ячейки инвентаря, а Storage хранит описания предметов. Т.е. Storage - изначально неправильное название сущности + класс занимается загрузкой данных, чем нарушает принцип единственной ответственности из SOLID и отсутствие инверсии зависимостей очень сильно усложняет тестирование такого кода. Почему это не репозиторий, который возвращает обычный массив?
Что такое null в классе UIInventoryCell? Есть же хотя бы шаблон Null Object, который упростит код в методе SetItem. То, как достается иконка в методе, тихий ужас.
DragContext. Как быть, если инвентарь пользователя взаимодействует с инвентарем ящика?
Не надо вызывать Instantiate в классе UIInventoryDragContainer. Зачем использовать Tuple, когда в Unity поддерживается C# с нормальными именованными кортежами, а не вот это вот Item1.
Зачем вводить IDynamicStorageActions, когда уже был обобщенный класс для хранения?
Не надо было в класс User добавлять в свойство вызов метода. Следовало в таком случае отказаться от свойства вообще. И не надо смешивать доменную модель с способом хранения: атрибут JsonProperty не к месту и нарушает принцип единственной ответственности в SOLID. Не надо делать такую жесть как сохранения пользователя на изменение инвентаря. Или возвращение false, когда ничего не удалось загрузить. Никто не вызовет метод Load, если загружать нечего.
Хороший ли это код? Ну не знаю, на мой взгляд код ужасен. Как-то ожидаешь лучшего от Master of Unity3D, а не статью на непонятном языке: "монобехи", "SO", ридонли, иммутабельно, стейт и так далее. И не решена изначальная проблема на самой первой картинке с особым размещением ячеек по нескольким сеткам.
camelCase — первое слово начинается со строчной буквы, каждое следующее слово с заглавной буквы, слова пишутся слитно;
PascalCase — каждое слово начинается с заглавной буквы, слова пишутся слитно;
kebab-case — все буквы строчные, слова отделяются символом "-";
snake_case — все буквы строчные, слова отделяются символом "_";
UPPER_CASE_SNAKE_CASE — все буквы заглавные, слова отделяются символом "_".
Один из моих любимых языков позволяет использовать апостроф и пробелы, что удобно в модульных тестах.
Про lower camelCase слышу впервые. Можно ссылку на источник?
Не согласен с утверждением «Функции далеко не всегда являются действиями», поскольку в примере приводится пример чего-то похожего на ленивые генераторы и конструктор типа, которые обычно именуются в форме глагола. Не сталкивался с хорошим обоснованием именовать функции в форме существутельных. Пример с созданием коллекции по умолчанию в js выглядит как вредный совет, поскольку при каждом вызове такой функции будет каждый раз создаваться новый объект.
Проблема в том, что нигде в статье не были описаны требования к инвентарю, кроме картинки, чтобы говорить о чем-то конкретном. В статье есть очень удачное предложение, которое характеризует большинство моих пунктов, что до ввода MonoBehaviour можно было играть в консоли. Они описывают тесную связь логики и представления с примесью статических классов. Никакие интерфейсы, абстракции и события в отделении логики от представления не помогут в этом случае, если их использовать таким образом. Код же необходимо писать таким образом, чтобы он был понятен другому разработчику, а не потому что и так все работает. Большую часть времени код читается, а не пишется.
В User нарушен SRP, потому что он управляет жизненным циклом инвентаря, управляет сохранением и загрузкой состояния, прибит гвоздями к конкретной реализации сериализации. Атрибут Serializable не требует наличие атрибута JsonProperty для сериализации классом JsonUtility, а сериализация классом JsonConvert не требует атрибут JsonProperty для сериализации всех свойств. Т.е. добавляется еще головная боль с тем, а всё ли можно засериализовать, и с тем, что нужно сериализовать, а что нет.
Касательно Null Object, речь про то, что у объекта два состояния: объект есть и объекта нет. Поведение с иконкой вылезло за границы объекта. Я знаю, что в Unity такое обычно решается через шаблон состояние.
Instantiate и зарабатывание миллионов долларов - связанные вещи. Мне так надоело, когда в том же Steam, появляется очередная игра усеянная багами, что иногда даже запустить ее невозможно. Конкретно здесь речь, когда условный тетрис (очень простая игра), отжирает огромное количество ресурсов или падает при странных условиях. Некоторые разработчики даже не удосуживаются банально проверять локаль системы, из-за чего падает DateTime, зато продают игру за 30 долларов. ААА игры тоже страдают от слабого уровня программистов, например Wasteland 3 долгое время грузил уровни по несколько минут, сбрасывал прогресс при загрузке сохранения. Главное верить, что озлобленные потребители не вернут деньги и не перестанут покупать будущие игры.
ScriptableObject обычно используется именно так, как я описал. Бывает даже в него домешивают поведение, никто же не запрещает. Был даже один довольно популярный доклад, который предлагал все на них делать. Такая веселая сущность, что даже сбрасывает все свои свойства при небольшом рефакторинге.
У меня статья вызывает больше недопонимания. Оговорюсь сразу, я очень далек от Unity.
Не существует никакого "идеального" инвентаря. Любая разработка начинается с определения набора требований. Как я должен понять требования к инвентарю по описанию эффекта Даннинга-Крюгера и некой непонятой картинке инвентаря? Что означает каждая секция инвентаря? Почему у ячейки инвентаря нет количества предметов?
Почему абстрактный класс BaseItem без ключевого слово abstract? Зачем вообще строить какую-то невнятную иерархию наследования без обозначения, кто его наследники? Какая вообще проблема решается через наследование? Почему просто не создать структуру только для чтения под названием InventoryCell (BaseItem - слишком абстрактное название)?
Почему ID у BaseItem задан как long? Я вижу сходу две явные реализации ID: как Enum (когда заранее известны все возможные предметы) и как еще одна структура только для чтения (когда все виды предметов загружаются динамически из ресурсов, когда надо валидировать ID и чтобы не дать возможность ID присвоить, скажем, CellID).
Почему количество ячеек в инвентаря определяется динамически, если на картинке нет полос прокруток?
Почему у Inventory ровно один интерфейс с очень странным названием IInventoryActions? Почему не просто IInventory? Зачем этот интерфейс вообще нужен, если в классе User он не используется? Почему IInventory не наследуется хотя бы от IEnumerable? Почему не используется индексатор для доступа к ячейке инвентаря? Зачем Add и Remove возвращают bool? Я должен поверить на слово? Это прямое нарушение YAGNI и принципа единственной ответственности из SOLID.
UIInventoryWindow. С какой целью абсолютно все поля во всех наследниках MonoBehaviour помечены атрибутом SerializeField, чтобы было, вдруг пригодится? Почему вообще вызывается Instantiate для ячейки, неужели нельзя обойтись без этого? Почему инвентарь не попадает в UIInventoryWindow через инверсию зависимости?
Почему UIInventoryCell содержит OnClickCell? Где вообще было сказано, что по ячейкам кликают мышкой? Почему OnClickCell не часть BaseItem? Почему у события неправильное название? OnCellClicked - это название для обработчика события, а не для события. Зачем вообще нужно это событие, какая проблема решается? У такой реализации событий очень много проблем, что проще забыть вообще про ключевое слово event. Одна из проблем прямо наглядно видна в методе OnPointerClick с ненужной проверкой на null.
Почему у класса User нет интерфейса? Почему приватное поле названо с большой буквы? Почему User знает как создавать инвентарь и самого себя? Почему не используется современный C# (или вовсе избавиться от приватного поля):
И что с того, что в играх на одного человека всего один игрок? Как этот код поддерживать? Как написать тесты на этот код? Про существование шаблона одиночка лучше вообще забыть.
ID чего принимает GetData в интерфейсе IStorageActions? Зачем здесь вообще обобщение используется?
В классе ItemVisualData (плохое название, класс описывает информацию о предмете) можно повесить атрибут SerializeField на свойство. Зачем вообще нужен этот атрибут? Для сериализации он не нужен. Почему VisualName - это строка, а не ID ресурса локализации? Статическое и только для чтения - это разные слова с разным смыслом, т.е. "статическое хранилище" - вообще не означает "только для чтения".
Зачем в классе ItemsVisualDataStorage метод Load? Не проще ли перенести содержимое метода в конструктор? И не надо в словаре вызывать метод ContainsKey, достаточно использовать индексатор.
ScriptableObject - это очень ужасное дизайнерское решение разработчиков Unity, что-то сопоставимое с шаблоном одиночка. Не надо в коде использовать сокращения, такие как SO. Почему эти данные не берутся из ресурсов?
Для тестов надо писать тесты, а не портить класс User.
Storage и Inventory. Раз приходится пояснять, почему это не одно и тоже, значит в коде что-то не так. У меня прослеживается логика, что Inventory хранит ячейки инвентаря, а Storage хранит описания предметов. Т.е. Storage - изначально неправильное название сущности + класс занимается загрузкой данных, чем нарушает принцип единственной ответственности из SOLID и отсутствие инверсии зависимостей очень сильно усложняет тестирование такого кода. Почему это не репозиторий, который возвращает обычный массив?
Что такое null в классе UIInventoryCell? Есть же хотя бы шаблон Null Object, который упростит код в методе SetItem. То, как достается иконка в методе, тихий ужас.
DragContext. Как быть, если инвентарь пользователя взаимодействует с инвентарем ящика?
Не надо вызывать Instantiate в классе UIInventoryDragContainer. Зачем использовать Tuple, когда в Unity поддерживается C# с нормальными именованными кортежами, а не вот это вот Item1.
Зачем вводить IDynamicStorageActions, когда уже был обобщенный класс для хранения?
Не надо было в класс User добавлять в свойство вызов метода. Следовало в таком случае отказаться от свойства вообще. И не надо смешивать доменную модель с способом хранения: атрибут JsonProperty не к месту и нарушает принцип единственной ответственности в SOLID. Не надо делать такую жесть как сохранения пользователя на изменение инвентаря. Или возвращение false, когда ничего не удалось загрузить. Никто не вызовет метод Load, если загружать нечего.
Хороший ли это код? Ну не знаю, на мой взгляд код ужасен. Как-то ожидаешь лучшего от Master of Unity3D, а не статью на непонятном языке: "монобехи", "SO", ридонли, иммутабельно, стейт и так далее. И не решена изначальная проблема на самой первой картинке с особым размещением ячеек по нескольким сеткам.
Один из моих любимых языков позволяет использовать апостроф и пробелы, что удобно в модульных тестах.
Про lower camelCase слышу впервые. Можно ссылку на источник?
Не согласен с утверждением «Функции далеко не всегда являются действиями», поскольку в примере приводится пример чего-то похожего на ленивые генераторы и конструктор типа, которые обычно именуются в форме глагола. Не сталкивался с хорошим обоснованием именовать функции в форме существутельных. Пример с созданием коллекции по умолчанию в js выглядит как вредный совет, поскольку при каждом вызове такой функции будет каждый раз создаваться новый объект.