# Проверка Barotrauma статическим анализатором PVS\-Studio

Barotrauma – игра, в которой можно поуправлять подлодкой, попрятаться от монстров и даже поиграть на аккордеоне в попытке не пойти ко дну\. Посмотрим, как проект, начатый инди\-студией Undertow Games и продолженный совместно с FakeFish, выглядит изнутри\. Для этого исследуем исходный код, преимущественно написанный на языке C\#, с помощью статического анализатора PVS\-Studio\.

![0930_Barotrauma_ru/image1.png](https://import.viva64.com/docx/blog/0930_Barotrauma_ru/image1.png)

## Введение 

Barotrauma – многопользовательский космический 2D\-симулятор подлодки в жанре survival horror\. Игроку предстоит управлять подлодкой, отдавать приказы, устранять протечки и противостоять опасностям\. 

Barotrauma не является Open Source проектом в обычном понимании\. Ранняя версия игры доступна бесплатно, текущая версия доступна в [Steam](https://store.steampowered.com/app/602960/Barotrauma/)\. Разработчики опубликовали исходный код на [GitHub](https://github.com/Regalis11/Barotrauma) для того, чтобы сообщество игроков могло свободно разрабатывать более комплексные моды и находить существующие ошибки\. 

## Результаты проверки

### Ошибки в if

[V3001](https://pvs-studio.ru/ru/docs/warnings/v3001/) There are identical sub\-expressions 'string\.IsNullOrEmpty\(EndPoint\)' to the left and to the right of the '\|\|' operator\. BanList\.cs 41

```cpp
public bool CompareTo(string endpointCompare)
{
  if (string.IsNullOrEmpty(EndPoint) || string.IsNullOrEmpty(EndPoint)) 
  { return false; }
  ....
}
```

Значение _EndPoint_ проверяется дважды\. Разработчик, скорее всего, забыл поменять параметр _EndPoint_ на _endpointCompare_ при копировании метода _string\.IsNullOrEmpty_\. Вообще программисты очень часто ошибаются в методах сравнения\. Рекомендую почитать [статью](https://pvs-studio.ru/ru/blog/posts/cpp/0509/) коллеги об этом\.

[V3004](https://pvs-studio.ru/ru/docs/warnings/v3004/) The 'then' statement is equivalent to the 'else' statement\. ServerEntityEventManager\.cs 314

```cpp
public void Write(Client client, IWriteMessage msg, 
                  out List<NetEntityEvent> sentEvents)
{
  List<NetEntityEvent> eventsToSync = null;
  if (client.NeedsMidRoundSync)
  {
    eventsToSync = GetEventsToSync(client);
  }
  else
  {
    eventsToSync = GetEventsToSync(client);
  }
  ....
}
```

Вне зависимости от значения _client\.NeedsMidRoundSync_ будет выполняться одно и то же\. Возможно следует убрать _else_\-ветвь или переработать её поведение\.

Следующий фрагмент кода вызвал два предупреждения:

* [V3021](https://pvs-studio.ru/ru/docs/warnings/v3021/) There are two 'if' statements with identical conditional expressions\. The first 'if' statement contains method return\. This means that the second 'if' statement is senseless DebugConsole\.cs 2177
* [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'args\.Length < 2' is always false\. DebugConsole\.cs 2183

```cpp
private static void InitProjectSpecific()
{
  ....
  AssignOnClientRequestExecute(
    "setclientcharacter",
    (Client senderClient, Vector2 cursorWorldPos, string[] args) =>
    {
      if (args.Length < 2)
      {
        GameMain.Server.SendConsoleMessage("....", senderClient);
        return;
      }

      if (args.Length < 2)
      {
        ThrowError("....");
        return;
      }
    );
  ....
}
```

Две одинаковые проверки\. В случае выполнения условия первого _if_ метод завершит работу, иначе же обе _then_\-ветви не будут выполнены\. 

При такой работе сообщение будет отправляться, но запись ошибки методом _ThrowError_ не произойдёт\. Следует объединить два тела _if_ или изменить условие второго\.

[V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'newPrice \> 0' is always true\. DebugConsole\.cs 3310

```cpp
private static void PrintItemCosts(....)
{
  if (newPrice < 1)
  {
    NewMessage(depth + materialPrefab.Name + 
    " cannot be adjusted to this price, because it would become less than 1.");
    return;
  }

  ....

  if (newPrice > 0)
  {
    newPrices.TryAdd(materialPrefab, newPrice);
  }
  ....
}
```

Если _newPrice_ меньше или равен 0, следует выполнение тела первого _if_\. После этого исполнение метода завершается\. Следовательно условие второго _if_ всегда будет истинно\. Поэтому можно добавить тело второго _if _в _else_\-ветвь первого или же вовсе его убрать\.  

### Опечатки

[V3005](https://pvs-studio.ru/ru/docs/warnings/v3005/) The 'arrowIcon\.PressedColor' variable is assigned to itself\. ChatBox\.cs 164

```cpp
public ChatBox(GUIComponent parent, bool isSinglePlayer)
{
  ....
  arrowIcon = new GUIImage(....)
  {
    Color = new Color(51, 59, 46)
  };
  arrowIcon.HoverColor = arrowIcon.PressedColor = 
  arrowIcon.PressedColor = arrowIcon.Color;
  ....  
}
```

Значение переменной _arrowIcon\.PressedColor _присваивается самой себе\. В то же время внутри класса _GUIIMage _содержится свойство _SelectedColor_\. Скорее всего, разработчик хотел использовать его, но опечатался\.

[V3005](https://pvs-studio.ru/ru/docs/warnings/v3005/) The 'Penetration' variable is assigned to itself\. Attack\.cs 324

```cpp
public Attack(float damage, 
              float bleedingDamage, 
              float burnDamage, 
              float structureDamage,
              float itemDamage, 
              float range = 0.0f, 
              float penetration = 0f)
{
   ....
   Range = range;
   DamageRange = range;
   StructureDamage = LevelWallDamage = structureDamage;
   ItemDamage = itemDamage;     
   Penetration = Penetration;                // <=
}
```

Ещё одна подобная ошибка\. В этом случае программист хотел проинициализировать свойства объекта, но вместо параметра _penetration_ присвоил свойству _Penetration_ своё же значение\. 

[V3025](https://pvs-studio.ru/ru/docs/warnings/v3025/) Incorrect format\. A different number of format items is expected while calling 'Format' function\. Arguments not used: t\.Character\.Name\. DebugConsole\.cs 1123

```cpp
private static void InitProjectSpecific()
{
  AssignOnClientRequestExecute("traitorlist", 
      (Client client, Vector2 cursorPos, string[] args) =>
  {
    ....
    GameMain.Server.SendTraitorMessage(
     client, 
     string.Format("- Traitor {0} has no current objective.",            // <=
                   "",                                                   // <=
                   t.Character.Name),                                    // <=
     "",
     TraitorMessageType.Console);   
  });
}
```

Исходя из смысла использованной в методе _GameMain\.Server\.SendTraitorMessage_ фразы, логично предположить, что спецификатор ввода _\{0\}_ должен был содержать _t\.Character\.Name_\. Однако же там окажется пустая строка\. 

Ошибка, скорее всего, является следствием неудачного copy\-paste предыдущего использования метода _GameMain\.Server\.SendTraitorMessage_:

```cpp
GameMain.Server.SendTraitorMessage(client, 
"There are no traitors at the moment.", "", TraitorMessageType.Console);
```

### Возможно возникновение NullReferenceException

[V3153](https://pvs-studio.ru/ru/docs/warnings/v3153/) Enumerating the result of null\-conditional access operator can lead to NullReferenceException\. Voting\.cs 181

```cpp
public void ClientRead(IReadMessage inc)
{
  ....
  foreach (GUIComponent item in
           GameMain.NetLobbyScreen?.SubList?.Content?.Children)    // <=
  {
    if (item.UserData != null && item.UserData is SubmarineInfo) 
    {
      serversubs.Add(item.UserData as SubmarineInfo); 
    }
  }
  ....
}
```

Если хотя бы один компонент из последовательности _GameMain\.NetLobbyScreen?\.SubList?\.Content?\.Children_ будет равен _null_, то результат всего выражения тоже будет равен _null_\. В таком случае будет выброшено исключение _NullReferenceException _при попытке перебора в _foreach_\.

Подробно про использование оператора _?_\. в _foreach_ можно прочитать в [этой статье](https://pvs-studio.ru/ru/blog/posts/csharp/0832/)\.

[V3027](https://pvs-studio.ru/ru/docs/warnings/v3027/) The variable 'spawnPosition' was utilized in the logical expression before it was verified against null in the same logical expression\. LevelObjectManager\.cs 274

```cpp
private void PlaceObject(LevelObjectPrefab prefab, 
                         SpawnPosition spawnPosition, 
                         Level level, Level.Cave parentCave = null)
{
  float rotation = 0.0f;
  if (   prefab.AlignWithSurface 
      && spawnPosition.Normal.LengthSquared() > 0.001f          // <=
      && spawnPosition != null)                                 // <=
  {
    rotation = MathUtils.VectorToAngle(new Vector2(spawnPosition.Normal.Y, 
                                                   spawnPosition.Normal.X));
  }
  ....
}
```

Из кода видно, что сначала идёт вызов метода _LengthSquared_ у поля _Normal_ переменной _spawnPosition_ и сравнение его с заданным значением, а затем переменная проверяется на _null_\.  Если _spawnPosition_ будет равна _null_, то возникнет исключение _NullReferenceException_\. 

Простейшим решением будет поместить проверку на _null_ в начало условия\. 

[V3095](https://pvs-studio.ru/ru/docs/warnings/v3095/) The 'level' object was used before it was verified against null\. Check lines: 107, 115\. BeaconMission\.cs 107

```cpp
public override void End()
{
  completed = level.CheckBeaconActive();                        // <=
  if (completed)
  {
    if (Prefab.LocationTypeChangeOnCompleted != null)
    {
      ChangeLocationType(Prefab.LocationTypeChangeOnCompleted);
    }
    GiveReward();
    if (level?.LevelData != null)                               // <=
    {
      level.LevelData.IsBeaconActive = true;
    }
  }
}
```

Сначала значение _level\.CheckBeaconActive_ присваивается, а затем используется оператор _?\._ в выражении _level?\.LevelData_\. В этом случае возможны ситуации, когда _level_ будет равен _null_ — и будет выброшено исключение _NullReferenceException_ либо _level_ никогда не будет _null —_ и проверка избыточна\.

### Выход за границы

[V3106](https://pvs-studio.ru/ru/docs/warnings/v3106/) Possibly index is out of bound\. The '0' index is pointing beyond 'Sprites' bound\. ParticlePrefab\.cs 303

```cpp
public ParticlePrefab(XElement element, ContentFile file)
{
  ....
  if (CollisionRadius <= 0.0f) 
    CollisionRadius = Sprites.Count > 0 ? 1 : 
                                          Sprites[0].SourceRect.Width / 2.0f;
}
```

При выполнении условия тернарного оператора значение переменной _CollisionRadius_ станет равно 1\. В противном случае значение _Sprites\.Count_ равно 0, и при обращении к первому элементу коллекции возникнет исключение _IndexOutOfRangeException_\.

Ранее по коду встречается проверка: является ли коллекция пустой\. 

```cpp
if (Sprites.Count == 0)
{
  DebugConsole.ThrowError($"Particle prefab \"{Name}\" in the file \"{file}\"
                            has no sprites defined!");
}
```

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

### Лишние действия

[V3107](https://pvs-studio.ru/ru/docs/warnings/v3107/) Identical expression 'power' to the left and to the right of compound assignment\. RelayComponent\.cs 150

```cpp
public override void ReceivePowerProbeSignal(Connection connection, 
                                             Item source, float power)
{
  ....
  if (power < 0.0f)
  {
    ....
  }
  else
  {
    if (connection.IsOutput || powerOut == null) { return; }

    if (currPowerConsumption - power < -MaxPower)
    {
      power += MaxPower + (currPowerConsumption - power);
    }
  }
}
```

Программист пытается прибавить переменной _power_ переменную _MaxPower_ и разницу между переменными _currPowerConsumption_ и _power_\. В разложенном варианте выражение будет иметь вид:

```cpp
power = power + MaxPower + (currPowerConsumption - power);
```

Выражение можно упростить, так как переменная _power_ вычитается сама из себя\. Упрощённый код будет выглядеть так: 



```cpp
power = MaxPower + currPowerConsumption;
```

### Всегда false

[V3009](https://pvs-studio.ru/ru/docs/warnings/v3009/) It's odd that this method always returns one and the same value of 'false'\. FileSelection\.cs 395

```cpp
public static bool MoveToParentDirectory(GUIButton button, object userdata)
{
  string dir = CurrentDirectory;
  if (dir.EndsWith("/")) { dir = dir.Substring(0, dir.Length - 1); }
  int index = dir.LastIndexOf("/");
  if (index < 0) { return false; }
  CurrentDirectory = CurrentDirectory.Substring(0, index+1);

  return false;
}
```

Странный метод, всегда возвращающий _false_\. Возможно, здесь нет ошибки и так задумано, либо один из _return_ должен возвращать _true_\.

### Утраченное значение 

[V3010](https://pvs-studio.ru/ru/docs/warnings/v3010/) The return value of function 'Trim' is required to be utilized\. GameServer\.cs 1589

```cpp
private void ClientWriteInitial(Client c, IWriteMessage outmsg)
{
  ....

  if (gameStarted)
  {
    ....

    if (ownedSubmarineIndexes.Length > 0)
    {
      ownedSubmarineIndexes.Trim(';');
    }
    outmsg.Write(ownedSubmarineIndexes);
  }
}
```

Метод _Trim_ не меняет значение _ownedSubmarineIndexes_, поэтому нет смысла вызывать его, не сохраняя результат\. Правильный вариант:

```cpp
ownedSubmarineIndexes = ownedSubmarineIndexes.Trim(';');
```

## Заключение

PVS\-Studio обнаружил ряд ошибок, опечаток и недочётов в исходном коде Baratrauma\. Многие из них довольно легко допустить при разработке и оставить незамеченными на code\-review\. 

Статический анализ может помочь разработчикам сохранить время, которое они бы потратили на поиск и исправление багов, и уделить его разработке нового контента\. Однако мало проверить код один раз\. Максимальная эффективность достигается при регулярном использовании анализатора в процессе написания кода\.

О проверке других проектов с помощью статического анализатора PVS\-Studio можно прочесть в нашем [блоге](https://pvs-studio.ru/ru/blog/)\.