Много статей написано про хороший и плохой код, но статей с разборами проблем реального кода очень мало (за исключением багов в open source проектах), поэтому решил показать проблемы в реальной игре на Unity.

Оригинальная версия игры вышла в 2016 году на японском, в 2023 на английском, сделана на Unity 5.1.4.
Для декомпиляции использовались dnSpy, ILSpy, dotPeek. Без ошибок не справился ни один, dotPeek оказался хуже всех, но в данном случае это говорит о качестве кода, а не о недостатках инструментов.

Некорректные элементы Enum

public enum EquipIndex
{
	Main Weapon,
	Sidearm,
	Headgear,
	Torso,
	Arms,
	Legs,
	Accessory,
	Food
}

У элемента "Main Weapon" посередине пробел, потому что EquipIndex связан с отображаемым игроку текстом.
Такой код не скомпилируется без специальных трюков, и локализация намертво прибита к коду, поэтому для каждого языка потребуется отдельная версия кода и исправление ошибок во всех версиях станет заметной проблемой.

Решение: в данных хранить значение Enum, а не имя, для локализации максимум использовать элементы как ключи.

Отключение EventSystem

Например так:

EventSystem.current.gameObject.SetActive(false);
// анимация или сохранение настроек
EventSystem.current.gameObject.SetActive(true);

С сохранением настроек и возникли проблемы: если зайти в настройки звука или боя и выйти ничего не поменяв, то при сохранение бросается NullReferenceException и игра "повисает" из-за отключенной EventSystem. И единственное решение это перезапуск игры, хотя критической ошибки нет.

Само сохранение настроек, когда в них ничего не поменялось, достаточно спорная вещь.
(Замечание: если хоть один параметр в настройках поменять, то всё работает без проблем.)

Решение: использовать CanvasGroup.interactable для переключения ограниченной группы элементов.

Ненужные coroutine в анимации интерфейса

private IEnumerator ShowMotion()
{
	GetComponent<RectTransform>().DOAnchorPos(new Vector2(-300f, 0f), 0.5f).SetEase(Ease.OutCubic);
	yield return new WaitForSeconds(0.5f);
	// завершающий код
}

Решение: использовать метод OnComplete, который вызывается при завершении анимации:

private void ShowMotion()
{
	GetComponent<RectTransform>().DOAnchorPos(new Vector2(-300f, 0f), 0.5f).SetEase(Ease.OutCubic).OnComplete(() => завершающий код);
}

Дополнительные проблемы:

• полсекунды на закрытие текущей панели, потом полсекунды на открытие новой это очень долго

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

• в некоторых местах длительность DOTween анимации и длительность ожидания (WaitForSeconds) разные, уверен что это ошибка и они должны быть одинаковыми

Интерфейсы разделены по сценам

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

Решение: сделать настраиваемую скорость анимаций интерфейса, не разделять интерфейсы по сценам или использовать аддитивную загрузку сцен.

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

На одно событие добавлено по 3-4 обработчика, для тестирования и прототипирования такое использовать можно, но в рабочем проекте это доставит проблем как минимум при рефакторинге и поиске использований.

Cмешивание бизнес-логики и представления

Код перемещения предмета из хранилища в инвентарь:

public void OnEndDragOutbox(coreButtonEventData ev)
{
	if (ev.selectTargetDrop == null || (!ev.selectTargetDrop.name.Equals("ItemBox") && ev.selectTargetDrop.name.IndexOf("inv") != 0))
	{
		return;
	}
	itemCView component = ev.selectTarget.GetComponent<itemCView>();
	if (inventoryControl.itemMaxCount(1) + 1 <= gameDatas.Instance.userData.GuildData.FGRMaxCount(1))
	{
		if (component.itemData.count[2] < 0)
		{
			component.itemData.count[2] = 0;
		}
		if (component.itemData.category == 23)
		{
			List<int> count;
			List<int> list = (count = component.itemData.count);
			int index;
			int index2 = (index = 1);
			index = count[index];
			list[index2] = index + component.itemData.count[2];
		}
		else
		{
			component.itemData.count[1] = component.itemData.count[2];
		}
		component.itemData.count[2] = 0;
		if (component.itemData.count[1] > 999)
		{
			component.itemData.count[1] = 999;
		}
		OnDrawUpdate();
		DrawUpdateHandler.CallDrawUpdate("ItemPanelGuild");
	}
}

Решение: вынести код перемещения предмета в отдельный метод MoveToInventory() и упростить метод.

public void OnEndDragOutbox(coreButtonEventData ev)
{
	if (ev.selectTargetDrop == null || (!ev.selectTargetDrop.name.Equals("ItemBox") && ev.selectTargetDrop.name.IndexOf("inv") != 0))
	{
		return;
	}
	itemCView component = ev.selectTarget.GetComponent<itemCView>();
	if (component.itemData.MoveToInventory())
	{
		OnDrawUpdate();
		DrawUpdateHandler.CallDrawUpdate("ItemPanelGuild");
	}
}

Неочевидные назначения полей

В коде выше используется поле List<int> count, в котором всегда три элемента. Индекс 2 это количество предметов в хранилище, 1 в инвентаре, а использование индекса 0 найти не удалось.

Решение: использовать два отдельных поля, или если необходима совместимость, то скрыть count и заменить свойствами, или хотя бы использовать именованные константы вместо 1 и 2.

const int STORAGE = 2;
const int INVENTORY = 1;

private int[] count;

public int QuantityStorage
{
	get => count[STORAGE];

	set => count[STORAGE] = value;
}

public int QuantityInventory
{
	get => count[INVENTORY];

	set => count[INVENTORY] = value;
}

Два публичных поля с одним смыслом

У класса предмета для поля name и unique. Оба отвечают за название предмета, только name это базовое неизменяемое название, а unique это полное название для предметов с суффиксом и префиксом.

Из-за этого была ошибка, когда при продаже простых предметов появлялось сообщение " was sold" без названия предмета.

base.informationLogControl.QueueDialog(string.Format(" was sold.", this.SallItem.unique));

Решение: сделать поля скрытыми и вместо них оставить одно свойство.

public string Name
{
	get => string.IsNullOrEmpty(unique) ? name : unique;
	
	set => unique = value;
}

Нестабильная сортировка предметов

Для сортировки используется List<T>.Sort(comparer), в документации к которому прямо говорится что сортировка нестабильная.
В результате при покупке, продаже или перемещении предметов порядок скачет. При тестировании такое выявить сложно, потому что при небольшом количестве элементов используется стабильная сортировка.

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

Отсутствие прокрутки списков

Отображение списков реализовано просто: фиксированное количество элементов без использования ScrollRect отображает часть элементов из списка. Но вот переключение между страницами только по нажатию кнопок.

Решение: добавить реализацию интерфейс IScrollHandler с методом OnScroll() для смены страниц.

public void OnScroll(PointerEventData eventData)
{
	PageObject2.pageMove(eventData.scrollDelta.y < 0 ? +1 : -1);
}

Дублирование кода

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

Решение: оставить один класс, а события (перетаскивание, клики и т.д.) перенаправлять родительской панели.

Экономия на символах

В корутинах часто используется

yield return 0;

0 короче null на три символа, но создает боксинг на пустом месте (возвращаемое значение нигде не используется).

Решение: использовать null.

Излишнее использование drag&drop

Покупка и продажа предметов, экипировка, крафт, и много прочего сделано через перетаскивание. Эти функции разделены по отдельным экранам, поэтому возможное действие только одно и можно обойтись двойным кликом.

Решение: метод OnPointerClick() имеет аргумент типа PointerEventData, достаточно проверить кнопку и количество нажатий.

var double_click = (eventData.button == PointerEventData.InputButton.Left)
	&& ((eventData.clickCount % 2) == 0);
if (double_click)
{
	// ...
}

Собственный Debug класс

public static class Debug
{
	static bool IsEnable()
	{
		return UnityEngine.Debug.isDebugBuild;
	}

	public static void Log(object message)
	{
		if (IsEnable())
		{
			UnityEngine.Debug.Log(message);
		}
	}
	
	// другие методы, аналогично Log
}

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

Решение: для методов добавить атрибут Conditional с символом для включения лога вместо использования IsEnable(), тогда вызовы методов будут удалены из билда.

[Conditional("DEBUG_MODE")]
public static void Log(object message)
{
	UnityEngine.Debug.Log(message);
}

Локализация в коде

ilc.QueueDialog("Obtained " + cnt + " " + itemName);
informationLogControl.QueueDialog(string.Format(" stone(s) synthesized.", selectItems.name));

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

Решение: использовать систему локализации.

Неоптимизированная загрузка ресурсов

TextAsset textAsset = binaryLoad.LoadFromAssetBundleExecute<TextAsset>(assetPath, filePath);
if (textAsset != null)
{
	TextAsset textAsset2 = UnityEngine.Object.Instantiate<TextAsset>(textAsset);
	MemoryStream memoryStream = new MemoryStream(textAsset2.bytes);
	BinaryFormatter binaryFormatter = new BinaryFormatter();
	obj = binaryFormatter.Deserialize(memoryStream);
	memoryStream.Close();
}

Вызов Instantiate() явно лишний, достаточно:

MemoryStream memoryStream = new MemoryStream(textAsset.bytes);

Множественная загрузка ресурсов

При загрузке сцены в цикле вызываются функции типа:

getItem = new ItemDataSet(gameDatas.loadDataSetDynamic<ItemDataSet>()[gets[0].getItem]);

вызов функции gameDatas.loadDataSetDynamic() в итоге сводится к:

FileStream fileStream = new FileStream(path, FileMode.Open, FileAccess.Read);
BinaryFormatter binaryFormatter = new BinaryFormatter();
object obj = binaryFormatter.Deserialize(fileStream);
fileStream.Close();

Загружаемый файл весит 541 килобайт, в итоге сцена загружается за пару секунд, после чего 5-20 секунд уходит на загрузку и десериализацию одного и тоже файла в цикле.
(вызов new ItemDataSet() создает копию полученного объекта, а не использует его как есть, поэтому проблем с множественными ссылками на один объект нет.)

Решение: кешировать загруженные данные и вынести создание объекта в отдельный метод.

public class ItemDataSet
{
	static List<ItemDataSet> cache;

	public static ItemDataSet Create(int index)
	{
		if (cache == null)
		{
			// загрузка и десериализация файла
		}
		
		return new ItemDataSet(cache[index]);
	}
}

getItem = ItemDataSet.Create(gets[0].getItem);

Клонирование объектов через сериализацию

public static T CloneObject<T>(T source)
{
	using MemoryStream memoryStream = new MemoryStream();
	BinaryFormatter binaryFormatter = new BinaryFormatter();
	binaryFormatter.Serialize(memoryStream, source);
	memoryStream.Position = 0L;
	return (T)binaryFormatter.Deserialize(memoryStream);
}

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

Решение: клонировать объект через MemberwiseClone() с последующим ручным клонированием для полей ссылочных типов.

Как минимум для списков структур без ссылок на изменяемые объекты можно создать отдельные версии без использования сериализации:

public static List<int> CloneObject(List<int> source)
{
	return new List<int>(source);
}

public static List<string> CloneObject(List<string> source)
{
	return new List<string>(source);
}

Использование классов вместо структур

public class LimitData
{
	public int physical;

	public int magical;
	
	// конструктор
}

Решение: в этом и многих других случаях структура уместнее, заодно снизит необходимость сериализации.

public struct LimitData
{
	public int physical;

	public int magical;
	
	// конструктор
}

Хранение ресурсов

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

Решение: хранить данные в ScriptableObject или JSON (если данные загружаются из внешнего источника или код должен работать вне Unity), по желания можно использовать AssetPostprocessor для конвертирования исходного файла в ScriptableObject.

Хранение текстур и спрайтов как TextAsset

public static byte[] LoadFromAssetBundleToBinary(string fn)
{
	string fileName = Path.GetFileName(fn);
	string assetPath = dataRoot + "/" + Path.GetDirectoryName(fn).ToLower() + ".unity3d";
	string filePath = dataRootAsA + "/" + Path.GetDirectoryName(fn) + "/" + fileName;
	TextAsset textAsset = LoadFromAssetBundleExecute<TextAsset>(assetPath, filePath);
	if (textAsset != null)
	{
		return textAsset.bytes;
	}
	return null;
}

Sprite sprite = null;
byte[] array = LoadFromAssetBundleToBinary(path + ".bytes");
if (array != null)
{
	Texture2D texture2D = new Texture2D(64, 64, TextureFormat.ARGB32, mipmap: false);
	texture2D.LoadImage(array);
	texture2D.filterMode = FilterMode.Trilinear;
	texture2D.Apply(updateMipmaps: true, makeNoLongerReadable: true);
	sprite = Sprite.Create(texture2D, new Rect(0f, 0f, texture2D.width, texture2D.height), new Vector2(pw, ph));
	string fileNameWithoutExtension = Path.GetFileNameWithoutExtension(path);
	sprite.name = fileNameWithoutExtension;
}

Большинство текстур хранятся как TextAsset в бандлах или вообще отдельным файлом, поэтому читаются байты, создается текстура, а из нее спрайт.

Решение: хранить текстуры и спрайты в бандлах именно как текстуры и спрайты, это не только упростит загрузку, но ещё снизит выделение памяти и уменьшит размер игры при настроенном сжатии текстур.

Обработка ввода разбросана по разным классам

if (Input.GetKeyDown(KeyCode.Backspace) && this.back.interactable)
{
	// ...
}

Решение: использовать InputSystem, если это возможно, или вынести проверки в отдельный класс, будет более читаемо и упростит переназначение клавиш.

public static class Actions
{
	public static bool Cancel()
	{
		return Input.GetKeyDown(KeyCode.Backspace);
	}
}

if (Actions.Cancel() && this.back.interactable)
{
	// ...
}

Непостоянность горячих клавиш

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

Два разных метода сохранения

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

Решение: сделать один публичный метод для сохранения, которому передавать информацию о текущем состоянии.

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

Использование неверной системы координат

В обучении маркеры (красный круг со стрелкой) часто указывают на неверную точку на скриншотах (на этой картинке это круг с мечом).

Координаты маркера заданы в 3D пространстве сцены, то есть для каждого маркера на новом скриншоте необходимо подбирать координаты в редакторе.

Решение: использовать координаты точки на скриншоте, и уже в коде конвертировать их в координаты маркера через Camera.ScreenToWorldPoint().

Использование объектов вместо компонентов

protected GameObject[] m_panels = new GameObject[6];

И дальше в коде используется в таком виде:

panels[i].GetComponent<unitBlockControl>();
panels[i].GetComponent<RectTransform>();

Решение: использовать нужный тип:

 protected unitBlockControl[] m_panels = new unitBlockControl[6];

где unitBlockControl будет хранить ссылку на свой RectTransform.

Итог

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