В прошлом уроке мы навели порядок в именах — класс pc стал PlayerCharacter, а загадочный Do() превратился в TakeDamage(). Код наконец-то заговорил по-человечески.
Вы создаёте pull request с первой игровой фичей — системой выдачи наград за убийство врагов. Метод GiveReward: игрок убивает монстра, получает золото. Казалось бы, что может пойти не так?
Через десять минут в PR появляется комментарий от Фёдора:
Фёдор: Четыре уровня вложенности, магические числа, двойное отрицание, загадочный булевый флаг... Давай разберём по шагам. Я тебе покажу, как из этого сделать конфетку.
Вот метод, на который Фёдор указывает:
public void GiveReward(Player player, Enemy enemy, bool isDouble)
{
if (player != null)
{
if (enemy.IsDead == true)
{
if (player.Level >= 10 && player.Level <= 50
&& enemy.Type != "boss" || player.HasVipPass)
{
if (isDouble)
player.Gold += enemy.Gold * 2;
else
player.Gold += enemy.Gold;
if (player.Gold > 999999)
player.Gold = 999999;
}
}
}
}
Выглядит как лестница, ведущая в подвал. Каждый if вложен в предыдущий, а чтобы понять, что метод делает, нужно мысленно пройти четыре уровня. Давайте исправлять — шаг за шагом, техника за техникой. К концу урока этот метод будет не узнать.
Начнём с самого болезненного — вложенности. Четыре уровня if превращают метод в «ёлочку», которую физически тяжело читать. Глаз прыгает вправо, пытается вспомнить, к какому if относится закрывающая скобка, теряется... и начинает сначала.
Представьте вышибалу на входе в ночной клуб. К нему подходит посетитель, и вышибала проверяет: нет паспорта — разворачивайся. Неподходящий дресс-код — разворачивайся. В чёрном списке — разворачивайся. Только тот, кто прошёл все проверки, попадает на танцпол. Вышибала не впускает всех внутрь, а потом разбирается — он отсекает на входе.
Именно так работают guard clauses — охранные проверки.
Применим эту технику к нашему GiveReward. Первые два if — это как раз проверки «на входе»: игрок не должен быть null, враг должен быть мёртв. Перевернём их:
public void GiveReward(Player player, Enemy enemy, bool isDouble)
{
if (player == null)
return;
if (enemy.IsDead != true)
return;
if (player.Level >= 10 && player.Level <= 50
&& enemy.Type != "boss" || player.HasVipPass)
{
if (isDouble)
player.Gold += enemy.Gold * 2;
else
player.Gold += enemy.Gold;
if (player.Gold > 999999)
player.Gold = 999999;
}
}
Уже лучше — два уровня вложенности исчезли. Метод стал читаться сверху вниз: нет игрока — уходим, враг жив — уходим. Дальше — основная логика. Но в коде всё ещё есть проблемы. Видите enemy.IsDead != true? Давайте разберёмся с этим.
Посмотрите на строку enemy.IsDead != true. Чтобы понять, что она означает, мозг проделывает двойную работу: сначала осмысливает «мёртв», потом инвертирует — «не мёртв... то есть жив». А если бы в коде было !isNotReady — пришлось бы инвертировать дважды. Это как фраза «я не не согласен» — формально верно, но зачем так мучить читателя?
Правило простое: выбирайте имена переменных так, чтобы условие можно было записать в положительной форме.
Вместо свойства IsDead (мёртв) лучше иметь IsAlive (жив). Тогда проверка «враг мёртв» записывается как !enemy.IsAlive — одно отрицание вместо двойной инверсии. А наша guard clause становится кристально ясной:
public void GiveReward(Player player, Enemy enemy, bool isDouble)
{
if (player == null)
return;
if (enemy.IsAlive)
return;
if (player.Level >= 10 && player.Level <= 50
&& enemy.Type != "boss" || player.HasVipPass)
{
if (isDouble)
player.Gold += enemy.Gold * 2;
else
player.Gold += enemy.Gold;
if (player.Gold > 999999)
player.Gold = 999999;
}
}
Фёдор: Видишь?
if (enemy.IsAlive) return— враг жив, награду не даём. Читается моментально. А теперь посмотри на эту длинную проверку внутри — вот где настоящая каша.
Фёдор прав. Посмотрите на это условие:
if (player.Level >= 10 && player.Level <= 50
&& enemy.Type != "boss" || player.HasVipPass)
Что здесь происходит? Уровень от 10 до 50, враг не босс... или есть VIP-пропуск? А какой приоритет у && и ||? Условие нужно не читать, а дешифровать. Когда выражение занимает больше двух-трёх логических операторов, самое время вынести его в отдельный метод с говорящим именем.
Создадим метод IsEligibleForReward — «имеет ли право на награду»:
private bool IsEligibleForReward(Player player, Enemy enemy)
{
if (player.HasVipPass)
return true;
bool isInLevelRange = player.Level >= MinRewardLevel
&& player.Level <= MaxRewardLevel;
bool isRegularEnemy = enemy.Type != "boss";
return isInLevelRange && isRegularEnemy;
}
public void GiveReward(Player player, Enemy enemy, bool isDouble)
{
if (player == null)
return;
if (enemy.IsAlive)
return;
if (!IsEligibleForReward(player, enemy))
return;
if (isDouble)
player.Gold += enemy.Gold * 2;
else
player.Gold += enemy.Gold;
if (player.Gold > 999999)
player.Gold = 999999;
}
Обратите внимание на две вещи. Во-первых, проверку на VIP-пропуск мы вынесли отдельно — это guard clause внутри вспомогательного метода. Во-вторых, сложные части условия получили имена: isInLevelRange, isRegularEnemy. Теперь каждая строка читается как обычное предложение, а не как математическая формула.
Заодно IsEligibleForReward стал ещё одной guard clause в основном методе — и вложенность снова уменьшилась.
Теперь посмотрим на числа в коде. Что такое 10? Что такое 50? Что за 999999? Через месяц даже автор кода не вспомнит, почему именно эти значения. Это магические числа — литералы без объяснения, вбитые прямо в логику.
Заменим их на именованные константы:
private const int MinRewardLevel = 10;
private const int MaxRewardLevel = 50;
private const int MaxGold = 999999;
private bool IsEligibleForReward(Player player, Enemy enemy)
{
if (player.HasVipPass)
return true;
bool isInLevelRange = player.Level >= MinRewardLevel
&& player.Level <= MaxRewardLevel;
bool isRegularEnemy = enemy.Type != "boss";
return isInLevelRange && isRegularEnemy;
}
public void GiveReward(Player player, Enemy enemy, bool isDouble)
{
if (player == null)
return;
if (enemy.IsAlive)
return;
if (!IsEligibleForReward(player, enemy))
return;
if (isDouble)
player.Gold += enemy.Gold * 2;
else
player.Gold += enemy.Gold;
if (player.Gold > MaxGold)
player.Gold = MaxGold;
}
Теперь MaxGold вместо загадочных 999999, а MinRewardLevel и MaxRewardLevel вместо голых 10 и 50. Если геймдизайнер решит изменить диапазон уровней — правка в одном месте, а не поиск по всему коду.
Фёдор: Кстати,
"boss"— это тоже магическая строка. В идеале тип врага стоит хранить в перечисленииEnemyType.Boss, а не в строке. Но это отдельная тема.
Остался последний штрих. Посмотрите на параметр bool isDouble. В месте вызова это выглядит так:
GiveReward(player, enemy, true);
Что значит true? Удвоенная награда? Двойной урон? Два врага? Без чтения сигнатуры метода — загадка. Булевый параметр-флаг — это всегда сигнал: что-то можно сделать яснее.
Заменим bool isDouble на int rewardMultiplier — множитель награды:
public void GiveReward(Player player, Enemy enemy, int rewardMultiplier)
{
if (player == null)
return;
if (enemy.IsAlive)
return;
if (!IsEligibleForReward(player, enemy))
return;
player.Gold += enemy.Gold * rewardMultiplier;
if (player.Gold > MaxGold)
player.Gold = MaxGold;
}
Два изменения в одном. Во-первых, вместо загадочного true в вызове теперь GiveReward(player, enemy, 2) — или 1 для обычной награды, 3 для тройной. Во-вторых, исчез целый if-else — ветвление заменилось простым умножением. Код стал и понятнее, и гибче.
И последнее — порядок проверок. Обратите внимание, как мы расставили guard clauses: сначала null-проверка (мгновенная), потом проверка состояния врага (одно свойство), и только потом — сложная проверка на право получения награды. От простого к сложному, от дешёвого к дорогому.
Это не просто эстетика. Если первая проверка отсекает большинство случаев — дальнейшие проверки даже не выполняются. Ставьте простые и быстрые проверки раньше сложных.
Давайте посмотрим, с чего мы начали — и к чему пришли.
Было:
public void GiveReward(Player player, Enemy enemy, bool isDouble)
{
if (player != null)
{
if (enemy.IsDead == true)
{
if (player.Level >= 10 && player.Level <= 50
&& enemy.Type != "boss" || player.HasVipPass)
{
if (isDouble)
player.Gold += enemy.Gold * 2;
else
player.Gold += enemy.Gold;
if (player.Gold > 999999)
player.Gold = 999999;
}
}
}
}
Стало:
private const int MinRewardLevel = 10;
private const int MaxRewardLevel = 50;
private const int MaxGold = 999999;
private bool IsEligibleForReward(Player player, Enemy enemy)
{
if (player.HasVipPass)
return true;
bool isInLevelRange = player.Level >= MinRewardLevel
&& player.Level <= MaxRewardLevel;
bool isRegularEnemy = enemy.Type != "boss";
return isInLevelRange && isRegularEnemy;
}
public void GiveReward(Player player, Enemy enemy, int rewardMultiplier)
{
if (player == null)
return;
if (enemy.IsAlive)
return;
if (!IsEligibleForReward(player, enemy))
return;
player.Gold += enemy.Gold * rewardMultiplier;
if (player.Gold > MaxGold)
player.Gold = MaxGold;
}
Тот же самый метод. Та же логика. Но вместо четырёх уровней вложенности — плоский список проверок. Вместо магических чисел — именованные константы. Вместо загадочного флага — понятный параметр. Вместо дешифровки — чтение.
Фёдор: Вот теперь — approve. Видишь, каждая техника сама по себе простая. Но вместе они превращают нечитаемую кашу в код, который можно открыть через полгода и сразу понять.
Guard clause (охранная проверка) — короткая проверка в начале метода, которая прерывает выполнение через return при некорректных данных. Убирает вложенность и делает метод плоским.
Двойные отрицания — конструкции вроде !isNotReady или IsDead != true. Выбирайте имена так, чтобы условие записывалось в положительной форме.
Вынесение условий — сложные логические выражения стоит разбивать на переменные с говорящими именами или выносить в отдельные методы (IsEligibleForReward()).
Магические числа — литералы без пояснения, вбитые в код. Заменяйте на именованные константы: MaxGold вместо 999999.
Булевые флаги в параметрах — bool isDouble в месте вызова превращается в загадочный true. Используйте параметры с говорящими именами или перечисления.
Порядок проверок — простые и быстрые проверки (на null) ставятся раньше сложных.