From 295fbf5f6f90146b5ad7c7deec539b3c417b981a Mon Sep 17 00:00:00 2001 From: gODealOAple Date: Sun, 30 Jun 2024 17:38:24 +0500 Subject: [PATCH] review2 --- Assets/Scripts/BackGroundLoop.cs | 2 +- Assets/Scripts/CameraController.cs | 5 ++++ Assets/Scripts/CasinoTrigger.cs | 8 +++++- Assets/Scripts/ChessTrigger.cs | 7 ++--- Assets/Scripts/GooseTrigger.cs | 6 ++++- Assets/Scripts/Inventory.cs | 28 +++++++++++++++++++- Assets/Scripts/LiftButtonTrigger.cs | 1 + Assets/Scripts/LiftController.cs | 1 + Assets/Scripts/Manager.cs | 1 + Assets/Scripts/OldLiftButtonTrigger.cs | 1 + Assets/Scripts/PlayerController.cs | 1 + Assets/Scripts/Rail trigger.cs | 2 +- Assets/Scripts/Sounds/GetVolumeByDistance.cs | 1 + Assets/Scripts/StartHintFadeOut.cs | 1 + 14 files changed, 57 insertions(+), 8 deletions(-) diff --git a/Assets/Scripts/BackGroundLoop.cs b/Assets/Scripts/BackGroundLoop.cs index 8cbf958..7b3da9d 100644 --- a/Assets/Scripts/BackGroundLoop.cs +++ b/Assets/Scripts/BackGroundLoop.cs @@ -14,7 +14,7 @@ private void Start() private IEnumerator PlayBackgroundSounds() { - while (true) + while (true) // review(30.06.2024): Не хватает способа выключить эти звуки, бесконечный цикл - не лучшая идея, стоило добавить хотя бы флажок какой-нибудь { foreach (var sound in backgroundSounds) { diff --git a/Assets/Scripts/CameraController.cs b/Assets/Scripts/CameraController.cs index ac4532f..85a4a7b 100644 --- a/Assets/Scripts/CameraController.cs +++ b/Assets/Scripts/CameraController.cs @@ -32,6 +32,10 @@ private void FindPlayer(bool playerIsLeft) { player = GameObject.FindGameObjectWithTag("CameraView").transform; lastX = Mathf.RoundToInt(player.position.x); + + // review(30.06.2024): Можно еще так было, кмк более читаемо + var direction = playerIsLeft ? Vector3.left : Vector3.right; + transform.position = player.position + direction * offset.x; transform.position = playerIsLeft ? new Vector3(player.position.x - offset.x, player.position.y, @@ -50,6 +54,7 @@ private void Update() isLeft = true; lastX = Mathf.RoundToInt(player.position.x); + // review(30.06.2024): Дублируется логика var target = isLeft ? new Vector3(player.position.x - offset.x, player.position.y, transform.position.z) diff --git a/Assets/Scripts/CasinoTrigger.cs b/Assets/Scripts/CasinoTrigger.cs index 378915c..ac6f49a 100644 --- a/Assets/Scripts/CasinoTrigger.cs +++ b/Assets/Scripts/CasinoTrigger.cs @@ -33,7 +33,8 @@ private void Update() { return; } - + + // review(30.06.2024): Зачем каждый раз проверять E? Может, стоило вынести метод типа if (E pressed) HandleCasinoActivating(); if (Input.GetKeyDown(KeyCode.E) && Inventory.Feather == 0 && meetingCount == 2 && PlayerController.Balance == 1) { if (!GoAway.isPlaying) @@ -47,6 +48,7 @@ private void Update() if (Input.GetKeyDown(KeyCode.E) && ((Inventory.Feather == 0 && meetingCount == 2) || PlayerController.Balance == 0)) { + // review(30.06.2024): Хорошо бы тут использовать метод AnySoundPlaying() if (TicketSound.isPlaying || DKRSound.isPlaying || MorsyankaSound.isPlaying || GoAway.isPlaying || casinoSound.isPlaying) return; ErrorSound.Play(); @@ -55,6 +57,7 @@ private void Update() if (Input.GetKeyDown(KeyCode.E) && PlayerController.Balance != 0 && meetingCount <= 2) { + // review(30.06.2024): Проверка дублируется if (TicketSound.isPlaying || DKRSound.isPlaying || MorsyankaSound.isPlaying || GoAway.isPlaying || casinoSound.isPlaying) return; casinoSound.Play(); @@ -67,6 +70,7 @@ private void Update() private IEnumerator StartTimer() { + // review(30.06.2024): Эта логика по прежнему дублируется isCounting = true; while (timeToCount > 0) { @@ -79,6 +83,7 @@ private IEnumerator StartTimer() timeToCount = 2f; switch (meetingCount) { + // review(30.06.2024): Интересно, почему в обратном порядке? case 2: Inventory.Morsynka = 1; MorsyankaSound.Play(); @@ -93,6 +98,7 @@ private IEnumerator StartTimer() break; } + // review(30.06.2024): Я бы изменил этот счетчик на enum CasinoStage, т.к. по замыслу это как будто ближе, да и более понятно, чем какое-то число meetingCount += 1; } diff --git a/Assets/Scripts/ChessTrigger.cs b/Assets/Scripts/ChessTrigger.cs index b2e2416..1db3e23 100644 --- a/Assets/Scripts/ChessTrigger.cs +++ b/Assets/Scripts/ChessTrigger.cs @@ -7,8 +7,8 @@ public class ChessTrigger : MonoBehaviour { private bool isPlayerNear; - private static int win; - private static int isPlay; + private static int win; // review(30.06.2024): bool? + private static int isPlay; // review(30.06.2024): Почему статичекое поле и почему не bool? public GameObject balance; public GameObject board; public GameObject boardOnLavka; @@ -20,7 +20,7 @@ public class ChessTrigger : MonoBehaviour public AudioSource audioSource; public AudioClip buttonClickSound; - public int gameState; + public int gameState; // review(30.06.2024): enum GameState, пусть даже приватный public float timeToCount; private bool isCounting; @@ -109,6 +109,7 @@ IEnumerator StartTimer() isPlay = 0; } + // review(30.06.2024): Тут как будто не хватает проверки на то, что это действительно игрок столкнулся private void OnTriggerEnter2D(Collider2D other) => isPlayerNear = true; private void OnTriggerExit2D(Collider2D other) => isPlayerNear = false; diff --git a/Assets/Scripts/GooseTrigger.cs b/Assets/Scripts/GooseTrigger.cs index b7196ef..8b00288 100644 --- a/Assets/Scripts/GooseTrigger.cs +++ b/Assets/Scripts/GooseTrigger.cs @@ -56,6 +56,7 @@ private void Start() { meetingCount = 0; + // review(30.06.2024): Код дублируется. Мне кажется, было бы уместно создать extension типа public static Color WithAlpha(this Color color, float alpha); Color color = feather.color; color.a = 0f; feather.color = color; @@ -86,6 +87,7 @@ private void Update() if (meetingCount == 2 && IsDKRInInventory) { IsDKRInInventory = false; + // review(30.06.2024): Дублирование кода из Start() var color = dkr.color; color.a = 0f; dkr.color = color; @@ -112,7 +114,7 @@ private void ShowItem() dkrView.enabled = true; IsDKRInInventory = true; StartCoroutine(FadeInDKR()); - GetComponent().Play(); + GetComponent().Play(); // review(30.06.2024): Наверное, имело смысл запросить этот компонент в Start() } if (Inventory.Ticket == 1) @@ -133,6 +135,7 @@ private void ShowItem() } } + // review(30.06.2024): Почти все FadeIn/Out дублируются. Стоило выделить один общий метод, а не дублировать так много IEnumerator FadeInDKR() { dkrView.color = new Color(dkrView.color.r, dkrView.color.g, dkrView.color.b, 0); @@ -196,6 +199,7 @@ IEnumerator FadeInMorsyanka() while (MorsynkaView.color.a < 1) { + // review(30.06.2024): Вот тут тоже бы пригодился extension WithAlpha MorsynkaView.color = new Color(MorsynkaView.color.r, MorsynkaView.color.g, MorsynkaView.color.b, MorsynkaView.color.a + Time.deltaTime); yield return null; } diff --git a/Assets/Scripts/Inventory.cs b/Assets/Scripts/Inventory.cs index bd4617f..2a4726b 100644 --- a/Assets/Scripts/Inventory.cs +++ b/Assets/Scripts/Inventory.cs @@ -1,7 +1,9 @@ +using System.Collections.Generic; using UnityEngine; -public class Inventory : MonoBehaviour +public class Inventory : MonoBehaviour // review(30.06.2024): Зачем наследование от монобеха? { + // review(30.06.2024): Мне кажется, что стоило инвентарную систему сделать чуть по-другому, т.к. у вас все предметы подразумеваются в единственном экземпляре, то лучше было бы использовать enum public static int Ticket; public static int Dkr; public static int Ship; @@ -19,3 +21,27 @@ private void Start() Key = 0; } } + +// review(30.06.2024): Я говорю про что-то такое +public static class Inventory_Refactored +{ + private static readonly HashSet Items = new(); + + public static bool Contains(Item item) => Items.Contains(item); + + public static void Add(Item item) => Items.Add(item); + + public static void Remove(Item item) => Items.Remove(item); + + public static IEnumerable GetAll() => Items; +} + +public enum Item +{ + Ticket, + Dkr, + Ship, + Morsynka, + Feather, + Key, +} diff --git a/Assets/Scripts/LiftButtonTrigger.cs b/Assets/Scripts/LiftButtonTrigger.cs index cae07e9..df54ce8 100644 --- a/Assets/Scripts/LiftButtonTrigger.cs +++ b/Assets/Scripts/LiftButtonTrigger.cs @@ -19,6 +19,7 @@ public class LiftButtonTrig : MonoBehaviour IEnumerator StartTimer() { + // review(30.06.2024): Очередное дублирование логики таймера isCounting = true; while (timeToCount > 0 && isCounting) { diff --git a/Assets/Scripts/LiftController.cs b/Assets/Scripts/LiftController.cs index 37116d6..3c8ce7a 100644 --- a/Assets/Scripts/LiftController.cs +++ b/Assets/Scripts/LiftController.cs @@ -12,6 +12,7 @@ public class LiftController : MonoBehaviour IEnumerator StartTimer() { + // review(30.06.2024): Слишком много комментариев. Они все не нужны isCounting = true; // Устанавливаем флаг, что таймер запущен while (timeToCount > 0) // Пока время не истечет { diff --git a/Assets/Scripts/Manager.cs b/Assets/Scripts/Manager.cs index d543d38..50146ac 100644 --- a/Assets/Scripts/Manager.cs +++ b/Assets/Scripts/Manager.cs @@ -46,6 +46,7 @@ public void Play() private IEnumerator FadeIn() { + // review(30.06.2024): Дублирование FadeIn-а var delta = 0.0f; while (delta < 1) { diff --git a/Assets/Scripts/OldLiftButtonTrigger.cs b/Assets/Scripts/OldLiftButtonTrigger.cs index 7f782eb..0e65560 100644 --- a/Assets/Scripts/OldLiftButtonTrigger.cs +++ b/Assets/Scripts/OldLiftButtonTrigger.cs @@ -2,6 +2,7 @@ using UnityEngine; using UnityEngine.SceneManagement; +// review(30.06.2024): Если он старый, то почему используется? public class OldLiftButtonTrigger : MonoBehaviour { public GameObject otherObject; diff --git a/Assets/Scripts/PlayerController.cs b/Assets/Scripts/PlayerController.cs index 5068f2f..a9425e4 100644 --- a/Assets/Scripts/PlayerController.cs +++ b/Assets/Scripts/PlayerController.cs @@ -75,6 +75,7 @@ private void Update() AdditionalTimer(); } + // review(30.06.2024): Непонятно, что такое Quantity, quantity чего? private void UpdateQuantity() { if (quantity != null) diff --git a/Assets/Scripts/Rail trigger.cs b/Assets/Scripts/Rail trigger.cs index 5ceae89..25f612b 100644 --- a/Assets/Scripts/Rail trigger.cs +++ b/Assets/Scripts/Rail trigger.cs @@ -35,7 +35,7 @@ private void Update() Player.SetActive(false); GetComponent().Play(); StartCoroutine(StartTimer()); - Player.transform.position = new Vector3(980, upY, -2); + Player.transform.position = new Vector3(980, upY, -2); // review(30.06.2024): Почему захардкожены константы? Может, стоило вынести весь вектор в поле? } if ((Input.GetKeyDown(KeyCode.S) || Input.GetKeyDown(KeyCode.DownArrow)) && !isLowestFloor) { diff --git a/Assets/Scripts/Sounds/GetVolumeByDistance.cs b/Assets/Scripts/Sounds/GetVolumeByDistance.cs index dd756a7..1bcbc5e 100644 --- a/Assets/Scripts/Sounds/GetVolumeByDistance.cs +++ b/Assets/Scripts/Sounds/GetVolumeByDistance.cs @@ -48,6 +48,7 @@ private void HandleSpeech(float distance) private void HandleBackgroundSound(float distance) { + // review(30.06.2024): Проверки дублируют те, что выше if (audioSource.isPlaying) return; diff --git a/Assets/Scripts/StartHintFadeOut.cs b/Assets/Scripts/StartHintFadeOut.cs index c770ede..e7aceb6 100644 --- a/Assets/Scripts/StartHintFadeOut.cs +++ b/Assets/Scripts/StartHintFadeOut.cs @@ -53,6 +53,7 @@ private IEnumerator WaitAndStartFadeOut(float waitTime) private IEnumerator FadeOutAnimation() { + // review(30.06.2024): var float elapsedTime = 0.0f; Color initialColor = spriteRenderer.color;