flexihtala · gODeaLoAple · Jun 26, 2024
diff --git a/Assets/Scripts/BreakBars.cs b/Assets/Scripts/BreakBars.cs
@@ -20,16 +20,16 @@ void Update()
         if (GameState.IsOverGameKeyboard
             && GameState.HaveCrowbar)
             interactableObject.isInteractable = true;
-        if (GameState.IsOverGameKeyboard
+        if (GameState.IsOverGameKeyboard // review(27.06.2024): не совсем понятно, почему тут эта проверка. Наверное, стоило написать что-то типа "CanInteract()" метода
             && GameState.HaveCrowbar
             && isTriggered
             && Input.GetKeyDown(KeyCode.E)
             && !GameState.CanOpenToiletDoor)
         {
-            audioManager.PlaySFX(audioManager.brokenBars);
+            audioManager.PlaySFX(audioManager.brokenBars); // review(27.06.2024): Т.к. тут используются поля audioManager, то стоило просто отдельный метод выделить, либо enum использовать
             brokenBars.SetActive(true);
             wholeBars.SetActive(false);
-            StartCoroutine(Wait(0.5f));
+            StartCoroutine(Wait(0.5f)); // review(27.06.2024): Магическая константа. Стоило вынести в поле
         }
     }
 
@@ -39,7 +39,8 @@ private IEnumerator Wait(float time)
         GameState.CanOpenToiletDoor = true;
         GameState.ChecksBool.Add(DialogFlagEnum.BrokenBars);
     }
-
+
+    // review(27.06.2024): Код в очередной раз дублируется
     private void OnTriggerEnter2D(Collider2D other)
     {
         if (!other.CompareTag("Player"))

diff --git a/Assets/Scripts/ConnectDots/DotsManager.cs b/Assets/Scripts/ConnectDots/DotsManager.cs
@@ -31,7 +31,7 @@ public class DotsManager : MonoBehaviour
     [SerializeField] private CanvasGroup tipsCanvas;
 
 
-    private readonly List<SpriteRenderer> CompletedTiles = new();
+    private readonly List<SpriteRenderer> CompletedTiles = new(); // review(17.06.2024): Приватные поля с маленькой буквы
     private bool isCoroutineStarted;
 
     private void Start()

diff --git a/Assets/Scripts/ConnectDots/DotsTile.cs b/Assets/Scripts/ConnectDots/DotsTile.cs
@@ -5,16 +5,16 @@
 public class DotsTile : MonoBehaviour
 {
     public DotsManager dotsManager;
-    public bool isHeadTile; 
+    public bool isHeadTile; // review(17.06.2024): Как будто isEmpty звучит понятнее
     private SpriteRenderer sprite;
     private Color endColor;
-    public List<GameObject> adjacentTiles;
+    public List<GameObject> adjacentTiles; // review(17.06.2024): Очень надеюсь, что вы не руками проставляли adjacentTiles... Кажется, что саму карту можно было построить из кода (что было бы даже проще)
 
     private void Awake()
     {
         sprite = GetComponent<SpriteRenderer>();
         dotsManager.defaultColor = sprite.color;
-        dotsManager.UncompletedTiles.Add(sprite);
+        dotsManager.UncompletedTiles.Add(sprite); // review(17.06.2024): Тут как раз нарушается MVP: вы основываете решение о незаконченных тайлах на основе их цвета, а не строите внутреннюю модель
         if (isHeadTile)
             endColor = transform.GetChild(0).GetComponent<SpriteRenderer>().color;
     }
@@ -27,6 +27,7 @@ private void OnMouseOver()
             if (dotsManager.isClicked)
             {
                 sprite.color = dotsManager.currentColor;
+                // review(17.06.2024): Всю логику dotsManager-а можно было бы запихнуть внутрь dotsManager.Click(gameObject)
                 dotsManager.CurrentTiles.Add(sprite);
                 dotsManager.prevTile = gameObject;
                 dotsManager.isStarted = true;
@@ -57,6 +58,7 @@ private void OnMouseOver()
             }
         }
 
+        // review(17.06.2024): Всю логику ниже стоило поместить в DotsManager
         if (dotsManager.CurrentTiles.Count > 1 && sprite == dotsManager.CurrentTiles[^2] && dotsManager.isClicked)
         {
             dotsManager.prevTile = dotsManager.CurrentTiles[^2].gameObject;

diff --git a/Assets/Scripts/ConnectDots/EraseButton.cs b/Assets/Scripts/ConnectDots/EraseButton.cs
@@ -7,7 +7,7 @@ public class EraseButton : MonoBehaviour
 {
     public DotsManager dotsManager;
 
-    void EraseField()
+    void EraseField() // review(17.06.2024): Метод не используется, можно удалить
     {
         if (!GameState.IsOverGameWires) 
             dotsManager.EraseField();

diff --git a/Assets/Scripts/ConnectDots/GlobalLightState.cs b/Assets/Scripts/ConnectDots/GlobalLightState.cs
@@ -7,6 +7,6 @@ public class GlobalLightState : MonoBehaviour
 {
     [SerializeField] private Light2D globalLight;
 
-    public void TurnOffLight() 
+    public void TurnOffLight() // review(17.06.2024): Метод не используется, можно удалить
         => globalLight.intensity = 0;
 }
diff --git a/Assets/Scripts/CraftItem.cs b/Assets/Scripts/CraftItem.cs
@@ -20,7 +20,7 @@ void Start()
     void Update()
     {
         var hasBrickAndNail = Inventory.PlayerInventory.ContainsKey(ItemName.Nail) &&
-                              Inventory.PlayerInventory.ContainsKey(ItemName.Brick);
+                              Inventory.PlayerInventory.ContainsKey(ItemName.Brick); // review(27.06.2024): Уместнее было инкапсулировать логику проверки вещей в метод Inventory.Contains(...)
         if (hasBrickAndNail)
             interactableObject.isInteractable = true;
 

diff --git a/Assets/Scripts/DoorPassage.cs b/Assets/Scripts/DoorPassage.cs
@@ -17,19 +17,22 @@ private void Start()
 
     private void Update()
     {
-        if (doorType == Door.RoomDoor && GameState.IsOverGameWires)
+        if (doorType == Door.RoomDoor && GameState.IsOverGameWires) // review(27.06.2024): Второй операнд лишний, наверное
             interactableObject.isInteractable = GameState.IsOverGameWires;
         if (doorType == Door.ToiletDoor)
             interactableObject.isInteractable = GameState.CanOpenToiletDoor;
         if (doorType == Door.Ventilation)
             interactableObject.isInteractable = GameState.HaveCrowbar;
         if (isTriggered && Input.GetKeyDown(KeyCode.E))
         {
+            // review(27.06.2024): Дублируется код. Тут имело смысл предсоздать объект(ы) типа DoorModel(Door Id, Func<bool> CanEnter)
+            // тогда упростилась бы и верхняя проверка, и эта
             if ((GameState.IsOverGameWires && doorType == Door.RoomDoor)
                 || (GameState.CanOpenToiletDoor && doorType == Door.ToiletDoor)
                 || (GameState.HaveCrowbar && doorType == Door.Ventilation))
             {
                 audioManager.PlaySFX(audioManager.door);
+                // review(27.06.2024): код ниже должен стать одним методом у Player
                 var vector3 = player.transform.position;
                 var position = exitDoor.transform.position;
                 vector3.x = position.x;

diff --git a/Assets/Scripts/ElectricalPanel.cs b/Assets/Scripts/ElectricalPanel.cs
@@ -17,7 +17,7 @@ private void Start()
 
     private void Update()
     {
-        interactableObject.isInteractable = GameState.ActivePlayer.penguinName == PenguinNames.Estriper;
+        interactableObject.isInteractable = GameState.ActivePlayer.penguinName == PenguinNames.Estriper; // review(27.06.2024): CanInteract(Player player)
         if (isTriggered && Input.GetKeyDown(KeyCode.E))
         {
             showGame = !showGame;
@@ -37,6 +37,7 @@ private void OnTriggerExit2D(Collider2D other)
         if (other.CompareTag("Player"))
         {
             isTriggered = false;
+            // review(27.06.2024): Все, что связано с dots стоило выделить в метод DotsManager
             dots.isClicked = false;
             dots.gameObject.SetActive(false);
             interactableObject.isInteractable = false;

diff --git a/Assets/Scripts/EndToilet.cs b/Assets/Scripts/EndToilet.cs
@@ -41,7 +41,8 @@ private void Update()
         inventory.SetActive(false);
 
         GameState.ActivePlayer.isActive = false;
-
+
+        // review(27.06.2024): Я бы создал дикт PenguinNames -> Animator и упростил логику
         if (GameState.ActivePlayer.penguinName != PenguinNames.Cago)
             CagoAnimator.SetBool("IsGameOver", true);
 
@@ -60,7 +61,7 @@ private void Update()
     private IEnumerator WaitTime()
     {
         GameState.ChecksBool.Add(DialogFlagEnum.End);
-        while (Math.Abs(EndCanvas.alpha - 1) >= 1e-9)
+        while (Math.Abs(EndCanvas.alpha - 1) >= 1e-9) // review(27.06.2024): Как будто не хватает extension-метода static bool IsEqualTo(this float number, float other, float accuracy = 1e-6)
         {
             EndCanvas.alpha += 0.005f;
             yield return null;

diff --git a/Assets/Scripts/GameState.cs b/Assets/Scripts/GameState.cs
@@ -5,20 +5,22 @@ public static class GameState
 {
     public static bool IsOverGameWires;
     public static bool IsOverGameKeyboard;
-    public static bool HaveCrowbar;
+    public static bool HaveCrowbar; // review(27.06.2024): HasCrowbar? А еще мб стоило честную инвентарную систему сделать?
     public static bool IsNowTextDisplayed;
     public static bool IsOpenKeyboardGame;
     public static bool CanOpenToiletDoor;
     public static Player ActivePlayer;
 
     public static Vector3 PlatformerSpawn = new(0 , 0, 0);
-    public static HashSet<DialogFlagEnum> ChecksBool = new()
+    public static HashSet<DialogFlagEnum> ChecksBool = new() // review(26.06.2024): Кмк более понятнее было бы ActivatedDialogs
     {
         DialogFlagEnum.Ventilation,
         DialogFlagEnum.None,
         DialogFlagEnum.Electrical,
         DialogFlagEnum.KeyboardForStupid
     };
 
-    public static AudioClip CurrentMusic;
+    // review(26.06.2024): Да, удобно, но кмк GameState - это бизнес-логика, а она не должна зависеть от моделей View (звуки - это тоже View)
+    // review(26.06.2024): Можно было бы завести enum под каждый стиль музыки и в классе создать дикт: enum -> AudioClip
+    public static AudioClip CurrentMusic; 
 }
diff --git a/Assets/Scripts/InteractableObject.cs b/Assets/Scripts/InteractableObject.cs
@@ -23,7 +23,7 @@ private void OnTriggerEnter2D(Collider2D other)
     {
         if (isInteractable && other.CompareTag("PlayerTrigger") && other.transform.parent.GetComponent<Player>().isActive)
         {
-            other.GetComponent<PlayerTrigger>().triggeredInteractableObjects.Add(gameObject);
+            other.GetComponent<PlayerTrigger>().triggeredInteractableObjects.Add(gameObject); // review(27.06.2024): Не хватает инкапсуляции логики
             sr.material = highlightMaterial;
         }
     }

diff --git a/Assets/Scripts/Inventory.cs b/Assets/Scripts/Inventory.cs
@@ -3,7 +3,7 @@
 using System.Collections.Generic;
 using UnityEngine;
 
-public class Inventory : MonoBehaviour
+public class Inventory : MonoBehaviour // review(27.06.2024): А зачем отнаследовали статический класс от MonoBehaviour?
 {
     public static Dictionary<ItemName, Item> PlayerInventory;
 

diff --git a/Assets/Scripts/InventoryUI.cs b/Assets/Scripts/InventoryUI.cs
@@ -7,15 +7,15 @@ public class InventoryUI : MonoBehaviour
 
     private void Start()
     {
-        Inventory.OnItemChangedCallback += UpdateUI;
+        Inventory.OnItemChangedCallback += UpdateUI; // review(27.06.2024): О, проявление MVC
         slots = transform.GetComponentsInChildren<InventorySlot>();
     }
 
     private void UpdateUI()
     {
         Debug.Log("Updating UI");
         Debug.Log(Inventory.PlayerInventory);
-        var playerInventoryValues = Inventory.PlayerInventory.Values.ToArray();
+        var playerInventoryValues = Inventory.PlayerInventory.Values.ToArray(); // review(27.06.2024): Inventory.GetAll() -> Item[]
         for (var i = 0; i < slots.Length; i++)
         {
             if (i < Inventory.PlayerInventory.Count)

diff --git a/Assets/Scripts/KeyboardGame/KBGameChecker.cs b/Assets/Scripts/KeyboardGame/KBGameChecker.cs
@@ -3,6 +3,7 @@
 using System.Collections.Generic;
 using UnityEngine;
 
+// review(26.06.2024): вроде как не используется, можно было бы удалить
 public class KBGameChecker : MonoBehaviour
 {
     private SpriteRenderer sprite;
@@ -14,6 +15,7 @@ private void Awake()
 
     private void Update()
     {
+        // review(26.06.2024): Каждый раз в update гонять проверку - накладно. Исключаем те случаи, когда вы навешиваете и убираете компонент
         if (GameState.IsOverGameKeyboard)
             sprite.color = Color.gray;
     }

diff --git a/Assets/Scripts/KeyboardGame/KeyboardGame.cs b/Assets/Scripts/KeyboardGame/KeyboardGame.cs
@@ -3,6 +3,7 @@
 using System.Collections.Generic;
 using UnityEngine;
 
+// review(26.06.2024): Вроде как класс называется KeyboardGame, а по факту он лишь включает/выключает игру. Я бы назвал его KeyboardGameSwitcher
 public class KeyboardGame : MonoBehaviour
 {
     public GameObject game;
@@ -17,17 +18,17 @@ private void Awake()
 
     private void Update()
     {
-        interactableObject.isInteractable = GameState.ActivePlayer.penguinName == PenguinNames.Kawazaki;
+        interactableObject.isInteractable = GameState.ActivePlayer.penguinName == PenguinNames.Kawazaki; // review(26.06.2024): стоило вынести Kawazaki в константы класса
         if (isTriggered && Input.GetKeyDown(KeyCode.E))
         {
-            game.SetActive(true);
+            game.SetActive(true); // review(26.06.2024): Вроде как создали InteractableObject, но все равно дублируете код активации. Возможно, имело смысл в InteractableObject положить Action, который должен выполниться
             GameState.IsOpenKeyboardGame = true;
         }
     }
 
     private void OnTriggerEnter2D(Collider2D other)
     {
-        if (other.CompareTag("Player") && other.GetComponent<Player>().penguinName == PenguinNames.Kawazaki)
+        if (other.CompareTag("Player") && other.GetComponent<Player>().penguinName == PenguinNames.Kawazaki) // review(26.06.2024): Повторение проверки. Имело смысл вынести в метод CanPlayKeyboardGame(Player player)
         {
             isTriggered = true;
         }

diff --git a/Assets/Scripts/KeyboardGame/TextPuzzle.cs b/Assets/Scripts/KeyboardGame/TextPuzzle.cs
@@ -6,6 +6,7 @@
 using TMPro;
 using UnityEngine.Serialization;
 
+// review(27.06.2024): Странно, что TextPuzzle находится в пространсте KeyboardGame
 public class TextPuzzle : MonoBehaviour
 {
     [SerializeField] private TMP_Text text;
@@ -17,16 +18,24 @@ public class TextPuzzle : MonoBehaviour
     [SerializeField] private CanvasGroup tipsCanvas;
     private string answer = "2 7 3 4";
 
-    private static List<KeyCode> Alphabet = new()
+    private static List<KeyCode> Alphabet = new() // review(26.06.2024): private static readonly IReadOnlyList<KeyCode> Alphabet
     {
-        KeyCode.Alpha0, KeyCode.Alpha1, KeyCode.Alpha2, KeyCode.Alpha3,
-        KeyCode.Alpha4, KeyCode.Alpha5, KeyCode.Alpha6, KeyCode.Alpha7,
-        KeyCode.Alpha8, KeyCode.Alpha9
+        KeyCode.Alpha0, 
+        KeyCode.Alpha1, 
+        KeyCode.Alpha2, 
+        KeyCode.Alpha3,
+        KeyCode.Alpha4, 
+        KeyCode.Alpha5, 
+        KeyCode.Alpha6, 
+        KeyCode.Alpha7,
+        KeyCode.Alpha8,
+        KeyCode.Alpha9
     };
 
     private IEnumerator EndGame()
     {
         RedLamp.SetActive(false);
+        // review(26.06.2024): Стоило в GameState выделить метод OnEndTextPuzzle() или типа того и уместить туда всю эту логику
         GameState.IsOverGameKeyboard = true;
         GameState.ChecksBool.Add(DialogFlagEnum.Keyboard);
         if (GameState.ChecksBool.Contains(DialogFlagEnum.Crowbar))
@@ -40,6 +49,7 @@ private IEnumerator EndGame()
 
     void Start()
     {
+        // review(26.06.2024): text ??= GetComponent<TMP_Text>();
         if (text == null)
             text = GetComponent<TMP_Text>();
     }
@@ -50,17 +60,18 @@ void Update()
             return;
         if (text.text == answer)
         {
-            StartCoroutine(EndGame());
+            StartCoroutine(EndGame()); // review(26.06.2024): это норма, что корутина будет стартовать каждый раз, когда будет ответ?
         }
 
         if (Input.GetKeyDown(KeyCode.Backspace) && text.text.Length > 0)
         {
             text.text = "";
-            keyAnimators[10].Play("PressKey");
+            keyAnimators[10].Play("PressKey"); // review(26.06.2024): Мне не очень нравится, что keyAnimators как бы зависят от Alphabet, но формально никак не связаны. Было бы круто выделить модель типа KeyboardKey(AudioClip Sound, KeyCode Code)
         }
 
         if (text.text.Length >= answer.Length)
             return;
+
         foreach (var el in Alphabet)
         {
             if (!Input.GetKeyDown(el)) continue;
@@ -72,5 +83,18 @@ void Update()
 
             keyAnimators[newDigit - '0'].Play("PressKey");
         }
+
+        // review(26.06.2024): решил написать свою версию метода. Возможно, она вам покажется читаемее
+        foreach (var el in Alphabet)
+        {
+            if (!Input.GetKeyDown(el)) continue;
+            var nexIndex = (Alphabet.IndexOf(el) + 2) % Alphabet.Count;
+            if (text.text.Length != 0)
+                text.text += " ";
+            else // review(26.06.2024): у этой сущности довольно сложный сеттер, поэтому стоит как можно реже к нему обращаться
+                text.text += " " + nexIndex;
+
+            keyAnimators[nexIndex].Play("PressKey");
+        }
     }
 }
diff --git a/Assets/Scripts/MusicChanger.cs b/Assets/Scripts/MusicChanger.cs
@@ -17,7 +17,7 @@ private void OnTriggerEnter2D(Collider2D other)
     {
         if (other.CompareTag("Player") && music != GameState.CurrentMusic)
         {
-            GameState.CurrentMusic = music;
+            GameState.CurrentMusic = music; // review(26.06.2024): Коль скоро вы задаете исходную музыку в самом audioManager, то стоило и изменение CurrentMusic поместить в audioManager.ChangeMusic
             audioManager.ChangeMusic(music);
         }
     }

diff --git a/Assets/Scripts/PlatformerRoom/Killer.cs b/Assets/Scripts/PlatformerRoom/Killer.cs
@@ -2,9 +2,25 @@
 
 public class Killer : MonoBehaviour
 {
+    // review(26.06.2024): очень много похожего кода. На вашем месте я бы выделил базовый класс 
+    // public abstract class PlayerTrigger : MonoBehaviour
+    // {
+    //     private void OnTriggerEnter2D(Collider2D other)
+    //     {
+    //         if (other.CompareTag("Player") && other.GetComponent<Player>() is {} player)
+    //         {
+    //             OnPlayerTriggerEnter2D(player);
+    //         }
+    //     }
+    //
+    //     public virtual void OnPlayerTriggerEnter2D(Player player)
+    //     {
+    //     }
+    // }
+
     private void OnTriggerEnter2D(Collider2D other)
     {
-        var unit = other.GetComponent<Player>();
+        var unit = other.GetComponent<Player>(); // review(26.06.2024): Хм, разве не нужно сначала проверить тег, а уже потом доставать компонент?
         if (unit && other.CompareTag("Player"))
         {
             unit.Die(unit);