SK0LX · gODeaLoAple · Jun 30, 2024
diff --git a/DarkPortal/Assets/C# scripts/ADtoMove.cs b/DarkPortal/Assets/C# scripts/ADtoMove.cs
@@ -1,6 +1,7 @@
 using System.Collections;
 using UnityEngine;
 
+// review(30.06.2024): Долго пытался понять, что значит название класса. Логичнее было бы назвать его MovementTutorial
 public class ADtoMove : MonoBehaviour
 {
     // Start is called before the first frame update

diff --git a/DarkPortal/Assets/C# scripts/Boss/BossEventsDie.cs b/DarkPortal/Assets/C# scripts/Boss/BossEventsDie.cs
@@ -1,5 +1,6 @@
 using UnityEngine;
 
+// review(29.06.2024): Класс используется? Если нет, то стоит удалить
 public class BossEvents : MonoBehaviour
 {
     private BoxCollider2D[] colliders;

diff --git a/DarkPortal/Assets/C# scripts/Boss/go.cs b/DarkPortal/Assets/C# scripts/Boss/go.cs
@@ -3,6 +3,7 @@
 using C__scripts.Enemies;
 using UnityEngine;
 
+// review(29.06.2024): Как будто класс не используется. Стоит удалять такое
 public class GoAnimation : StateMachineBehaviour
 {
     [SerializeField] public bool MoveLeft;

diff --git a/DarkPortal/Assets/C# scripts/Camera.cs b/DarkPortal/Assets/C# scripts/Camera.cs
@@ -31,6 +31,8 @@ private void Start()
 
     private void LateUpdate()
     {
+        // review(30.06.2024): Кмк не самый лучший способ опеделять локацию. Для этого больше подойдет какое-то поле, которое бы изменялось при перемещении игрока в ту или иную локацию
+        // review(30.06.2024): А еще тут лучше было бы использовать enum вместо циферок
         if (player.position.x < rightLimit1.position.x && player.position.x > leftLimit1.position.x)
             location = 1;
         if (player.position.x < rightLimit2.position.x && player.position.x > leftLimit2.position.x)

diff --git a/DarkPortal/Assets/C# scripts/Dialoges/Dialog.cs b/DarkPortal/Assets/C# scripts/Dialoges/Dialog.cs
@@ -9,6 +9,7 @@ public class Dialog
     public Message[] message;
 }
 
+// review(29.06.2024): Стоит вынести в отдельный файл
 [System.Serializable]
 public class Message
 {

diff --git a/DarkPortal/Assets/C# scripts/Dialoges/DialogForMobs.cs b/DarkPortal/Assets/C# scripts/Dialoges/DialogForMobs.cs
@@ -1,5 +1,7 @@
+using System;
 using System.Collections;
 using System.Collections.Generic;
+using System.Linq;
 using JetBrains.Annotations;
 using TMPro;
 using UnityEngine;
@@ -19,16 +21,28 @@ public class DialogForMobs : MonoBehaviour
     public TriggetText triggetDialogue3;
     public TriggetText bossDialog;
     private int countDialog;
+    private Dialog[] dialogs;
 
     public Player player;
 
+    public void Start()
+    {
+        dialogs = new[]
+        {
+            new Dialog(triggetDialogue1, CanvasForMob1, name1, text1, disableCanvasOnSkip: true),
+            new Dialog(triggetDialogue2, CanvasForMob1, name1, text1),
+            new Dialog(triggetDialogue3, CanvasForMob1, name1, text1),
+            new Dialog(bossDialog, boss, bossName, bossText, disableCanvasOnSkip: true),
+        };
+    }
+
     public void StartMessage()
     {
         switch (countDialog)
         {
             case 0:
                 triggetDialogue1.TriggerDialog(CanvasForMob1, name1, text1);
-                countDialog++;
+                countDialog++; // review(30.06.2024): Как минимум, увеличение счетчика можно вынести из switch
                 break;
             case 1:
                 triggetDialogue2.TriggerDialog(CanvasForMob1, name1, text1);
@@ -49,7 +63,7 @@ public bool EndDialog()
     {
         if (triggetDialogue1.end)
         {
-            CanvasForMob1.enabled = false;
+            CanvasForMob1.enabled = false; // review(30.06.2024): Не совсем понятно, почему только в этом случае и при боссе отключается канвас
             triggetDialogue1.end = false;
             return true;
         }
@@ -76,4 +90,56 @@ public bool EndDialog()
         return false;
     }
 
+    // review(30.06.2024): Примерно такой код хотелось бы видеть вместо захардкоженных значений. Он более удобный и расширяемый
+    public void StartMessage_Refactored()
+    {
+        if (countDialog >= dialogs.Length)
+            return;
+
+        var dialog = dialogs[countDialog];
+
+        dialog.Run();
+
+        countDialog++;
+    }
+
+    public bool EndDialog_Refactored()
+    {
+        var skipRequiredDialog = dialogs.FirstOrDefault(x => x.SkipRequired);
+
+        if (skipRequiredDialog is null)
+            return false;
+
+        skipRequiredDialog.AcceptSkip();
+        return true;
+    }
+
+    private class Dialog
+    {
+        private readonly TriggetText trigger;
+        private readonly Canvas canvas;
+        private readonly TextMeshProUGUI name;
+        private readonly TextMeshProUGUI text;
+        private readonly bool disableCanvasOnSkip;
+
+        public Dialog(TriggetText trigger, Canvas canvas, TextMeshProUGUI name, TextMeshProUGUI text, bool disableCanvasOnSkip = false)
+        {
+            this.trigger = trigger;
+            this.canvas = canvas;
+            this.name = name;
+            this.text = text;
+            this.disableCanvasOnSkip = disableCanvasOnSkip;
+        }
+
+        public bool SkipRequired => trigger.end;
+
+        public void AcceptSkip()
+        {
+            if (disableCanvasOnSkip)
+                canvas.enabled = false;
+            trigger.end = false;
+        }
+
+        public void Run() => trigger.TriggerDialog(canvas, name, text);
+    }
 }
diff --git a/DarkPortal/Assets/C# scripts/Dialoges/DialogManager.cs b/DarkPortal/Assets/C# scripts/Dialoges/DialogManager.cs
@@ -6,14 +6,15 @@
 using UnityEngine;
 using UnityEngine.UI;
 
+// review(30.06.2024): Я обычно избегаю абстрактных названий типа Manager/Controller. Эта штука больше похожа на DialogPlayer
 public class DialogManager : MonoBehaviour
 {
     private Queue<Message> sentences;
     private TextMeshProUGUI dialogText;
     private TextMeshProUGUI nameText;
     private Image image;
     private Coroutine typingCoroutine;
-    [SerializeField] private AudioSource text;
+    [SerializeField] private AudioSource text; // review(30.06.2024): Я бы назвал это textSound, а то текущее название как будто подразумевает, что это какая-то строка
 
     private void Start()
     {
@@ -35,6 +36,7 @@ public void StartDialogue(Dialog dialog, Canvas canvas, TextMeshProUGUI name, Te
 
     }
 
+    // review(30.06.2024): Не очень понятно, что означает bool. Лучше было бы создать enum типа DialogStatus { NotStarted, Executing, Finished }, чтобы извне класса было понятно, что означает результат действия метода
     public bool DisplayNextSentence()
     {
         if (sentences.Count == 0)
@@ -46,21 +48,26 @@ public bool DisplayNextSentence()
             text.Stop();
             return true;
         }
+
+        // review(30.06.2024): код бы чуть-чуть отформатировать, чтобы выделить блоки логики
         var message = sentences.Dequeue();
         if (image is null)
         {
-            image = message.imagePerson;
+            image = message.imagePerson; // review(30.06.2024): Эту строку можно вынести после блока if, тогда он сократится вообще до одного if, при котором будет image.enabled = false
         }
         else
         {
             image.enabled = false;
             image = message.imagePerson;
         }
-
+
+        // review(30.06.2024): Код по Reset-у отображения диалога как будто стоит выделить в отдельный метод, а то он дублируется здесь и выше
         if (typingCoroutine != null)
         {
             StopCoroutine(typingCoroutine);
         }
+
+        // review(30.06.2024): Как будто тут еще надо выключить text на всякий случай
         typingCoroutine = StartCoroutine(TypeSentence(message));
         return false;
     }

diff --git a/DarkPortal/Assets/C# scripts/Dialoges/FirstDialog.cs b/DarkPortal/Assets/C# scripts/Dialoges/FirstDialog.cs
@@ -18,7 +18,7 @@ private void Start()
         canvasForFirstItteration.enabled = true;
         player.canvasDefault.enabled = false;
         triggerDialogue.TriggerDialog(canvasForFirstItteration, name, text);
-        player.speed = 0f;
+        player.speed = 0f; // review(30.06.2024): Кажется, более описательным было бы использование метода player.Freeze()
     }
 
     private void Update()
@@ -27,7 +27,7 @@ private void Update()
         {
             canvasForFirstItteration.enabled = false;
             player.canvasDefault.enabled = true;
-            player.speed = 5f;
+            player.speed = 5f; // review(30.06.2024): Почему именно 5? Выглядит как магическая константа
             dialogTriggered = false;
             triggerDialogue.end = false;
         }

diff --git a/DarkPortal/Assets/C# scripts/Dialoges/triggetText.cs b/DarkPortal/Assets/C# scripts/Dialoges/triggetText.cs
@@ -4,6 +4,7 @@
 using UnityEngine;
 using UnityEngine.UI;
 
+// review(29.06.2024): Разве не TriggerText?
 public class TriggetText : MonoBehaviour
 {
     public Dialog dialog;
@@ -17,6 +18,7 @@ private void Start()
 
     public void TriggerDialog(Canvas canvas, TextMeshProUGUI name, TextMeshProUGUI text)
     {
+        // review(29.06.2024): Может, лучше найти DialogManager единожды при Start?
         FindObjectOfType<DialogManager>().StartDialogue(dialog, canvas, name, text);
     }
 

diff --git a/DarkPortal/Assets/C# scripts/Enemies/EnemiesManager.cs b/DarkPortal/Assets/C# scripts/Enemies/EnemiesManager.cs
@@ -3,6 +3,7 @@
 
 namespace C__scripts.Enemies
 {
+    // review(30.06.2024): Это больше похоже на EnemiesSpawner
     public class EnemiesManager : MonoBehaviour
     {
         [SerializeField] private GameObject player;
@@ -27,17 +28,19 @@ public void Start()
 
         public void Update()
         {
+            // review(30.06.2024): Если инвертировать условие, то уменьшится вложенность
             if (Vector3.Distance(spawnPoints[currentSpawnPoint].position, player.transform.position) < 10f)
             {
                 if (!isFullTimeSpawn)
                 {
                     enemies[currentSpawnPoint] = SpawnEnemy(spawnPoints[currentSpawnPoint], enemyPrefab[currentSpawnPoint]);
                     currentSpawnPoint++;
-                    if (currentSpawnPoint >= size) 
+                    if (currentSpawnPoint >= size)
                         Destroy(gameObject);
                 }
                 else
                 {
+                    // review(30.06.2024): Такие сложные условия я бы выносил в методы (в плане один метод ShouldSpawnWithTime()
                     if (Vector3.Distance(spawnPoints[currentSpawnPoint].position, player.transform.position) > 5f
                         && player.transform.position.x < spawnPoints[currentSpawnPoint].position.x
                         && enemies[currentSpawnPoint] == null)
@@ -50,7 +53,8 @@ public void Update()
         private void SpawnWithTime()
         {
             enemies[currentSpawnPoint] = SpawnEnemy(spawnPoints[currentSpawnPoint],
-                enemyPrefab[random.Next(0, enemyPrefab.Length)]);
+                enemyPrefab[random.Next(0, enemyPrefab.Length)]); // review(30.06.2024): Я бы вынес получение индекса в метод либо хотя бы в переменную, чтобы было проще читать
+            // review(30.06.2024): можно упростить до currentSpawnPoint = (currentSpawnPoint + 1) % spawnPoints.Length;
             currentSpawnPoint++;
             if (currentSpawnPoint == spawnPoints.Length)
                 currentSpawnPoint = 0;

diff --git a/DarkPortal/Assets/C# scripts/Enemies/GamePlay.cs b/DarkPortal/Assets/C# scripts/Enemies/GamePlay.cs
@@ -2,6 +2,7 @@
 
 namespace C__scripts.Enemies
 {
+    // review(30.06.2024): Не совсем понятно, как используется класс
     public class GamePlay: MonoBehaviour
     {
         [SerializeField] private GameObject enemyPrefab;

diff --git a/DarkPortal/Assets/C# scripts/Enemies/GoAnimationMob.cs b/DarkPortal/Assets/C# scripts/Enemies/GoAnimationMob.cs
@@ -13,8 +13,10 @@ public class GoAnimationMob : StateMachineBehaviour
     private float radius;
 
     // OnStateEnter is called when a transition starts and the state machine starts to evaluate this state
+    // review(30.06.2024): public override
     override public void OnStateEnter(Animator animator, AnimatorStateInfo stateInfo, int layerIndex)
     {
+        // review(30.06.2024): Почему бы animator.GetComponent<Enemy>() не вынести в переменную?
         spawnPosition = animator.GetComponent<Enemy>().SpawnPoint.position;
         radius = animator.GetComponent<Enemy>().Radius;
         speed = animator.GetComponent<Enemy>().Speed;
@@ -26,7 +28,7 @@ override public void OnStateEnter(Animator animator, AnimatorStateInfo stateInfo
     // OnStateUpdate is called on each Update frame between OnStateEnter and OnStateExit callbacks
     override public void OnStateUpdate(Animator animator, AnimatorStateInfo stateInfo, int layerIndex)
     {
-        var x = default(float);
+        var x = default(float); // review(30.06.2024): var x = 0f; 
         if (isMoveRight)
         {
             x = spawnPosition.x + radius;

diff --git a/DarkPortal/Assets/C# scripts/Enemies/mob.cs b/DarkPortal/Assets/C# scripts/Enemies/mob.cs
@@ -61,6 +61,7 @@ public void Init(GameObject gameObject, Transform spawnPosition, GameObject play
         public void FixedUpdate()
         {
             if (gameObject is null) return;
+            // review(30.06.2024): А зачем изменять состояние в цикле? Разве он не поменяется на Move и останется в этом состоянии постоянно?
             switch (State)
             {
                 case EnemyState.Born:
@@ -73,7 +74,7 @@ public void FixedUpdate()
 
         public IEnumerator Attack()
         {
-            if (new Random().Next(0, 101) < 20)
+            if (new Random().Next(0, 101) < 20) // review(30.06.2024): Хорошо было бы добавить extension типа Random.FlipCoin (а еще от констант избавиться)
                 entity.UseSkills();
             var geolocationNow = transform.position.x;
             var geolocationPlayer = player.transform.position.x + boxRadius + playerBox;
@@ -98,7 +99,7 @@ public IEnumerator Attack()
                 transform.position += new Vector3(speed * Time.deltaTime, 0, 0);
                 yield return null;
             }
-            transform.eulerAngles = new Vector3(0, -180, 0);
+            transform.eulerAngles = new Vector3(0, -180, 0); // review(30.06.2024): Часто встречаю эту конструкцию. Я бы выделил extension-метод, который бы описывал, зачем изменяется ротация, а то сейчас не совсем понятно
             yield return new WaitForSeconds(0.1f);
             animator.SetTrigger(Idle);
         }
@@ -112,7 +113,7 @@ public IEnumerator Die()
             else
             {
                 yield return new WaitForSeconds(1f);
-                var canvas = Instantiate(gameObject.GetComponent<Boss>().canvasWIN);
+                var canvas = Instantiate(gameObject.GetComponent<Boss>().canvasWIN); // review(30.06.2024): Подозреваю, что лучше сохранить ссылку на объект, иначе он может быть собран сборщиком мусора
             }
         }
 
@@ -133,10 +134,38 @@ public void OnTriggerEnter2D(Collider2D other)
 
                 transform.position += new Vector3(2f, 0);
             }
-            else if (other.CompareTag("Player") && State is EnemyState.Move)
+            else if (other.CompareTag("Player") && State is EnemyState.Move) // review(30.06.2024): Условие частично дублируется, можно внутри предыдущего if сделать проверку
             {
                 StartCoroutine(PrepareToFight());
             }
+
+            // review(30.06.2024): имею в виду такое
+
+            void Method()
+            {
+                if (!other.CompareTag("Player") || State is not EnemyState.Move)
+                    return;
+
+                if (isBoss)
+                {
+                    StartCoroutine(PrepareToFight());
+                    return;
+                }
+
+                // review(30.06.2024): А почему это не выделили в метод, как PrepareToFight?
+                Destroy(GetComponent<BoxCollider2D>());
+                State = EnemyState.Fight;
+                player.speed = 0;
+                player.fight = true;
+
+                fight = Instantiate(fight);
+                fight.Init(player.gameObject, canvasForFight, gameObject);
+                entity.ShowCanvas();
+                animator.ResetTrigger(Go);
+                animator.SetTrigger(Idle);
+
+                transform.position += new Vector3(2f, 0);
+            }
         }
 
         private IEnumerator PrepareToFight()
@@ -153,7 +182,8 @@ private IEnumerator PrepareToFight()
             State = EnemyState.Fight;
 
             animator.SetBool("isFight", true);
-
+
+            // review(30.06.2024): Дублируется логика
             fight = Instantiate(fight);
             fight.Init(player.gameObject, canvasForFight, gameObject);
             player.fightObject = fight;

diff --git a/DarkPortal/Assets/C# scripts/Entity.cs b/DarkPortal/Assets/C# scripts/Entity.cs
@@ -65,6 +65,7 @@ public void ShowCanvas()
 
     private void UpdateCanvasHp()
     {
+        // review(30.06.2024): Уже видел такой код в HealthBar. Кажется, тут его стоило переиспользовать
         bar1.fillAmount = (float)health / maxHealth;
         text.text = $"{health}/{maxHealth}";
         if (health <= 0)