Refactoring Code by NemesisWar · Pull Request #38 · HolyMonkey/SHK-Unity

NemesisWar · 2021-04-15T16:42:14Z

Styled the code for standards. Changed script names. Changed the content of the scripts.

Add RequireComponents in scripts

HolyMonkey · 2021-04-22T15:54:49Z

+
+    private void OnMove(Vector2 move)
+    {
+        transform.position += new Vector3(move.x*_speed, move.y*_speed, 0) * Time.deltaTime;


Вынеси умножение на _speed из консруктора

HolyMonkey · 2021-04-22T15:55:47Z

+[RequireComponent(typeof(BoxCollider2D))]
+public class Enemy : MonoBehaviour
+{
+    public UnityAction<Enemy> DeadEnemy;


https://stackoverflow.com/questions/150479/order-of-items-in-classes-fields-properties-constructors-methods

HolyMonkey · 2021-04-22T15:56:33Z

+public class Enemy : MonoBehaviour
+{
+    public UnityAction<Enemy> DeadEnemy;
+    [SerializeField] private int _radiusNewPosition;


_radius
и может всё-таки float?

HolyMonkey · 2021-04-22T15:56:47Z

+    public UnityAction<Enemy> DeadEnemy;
+    [SerializeField] private int _radiusNewPosition;
+    [SerializeField] private float _speed;
+    private Vector2 _targetPosition;


HolyMonkey · 2021-04-22T15:57:12Z

+        }
+    }
+
+    private void SetNewTransformPosition()


Set неудачный глагол. Почитай про CQRS и что означает Set в этой терминологии

HolyMonkey · 2021-04-22T15:57:53Z

+{
+    public UnityAction StopGame;
+    [SerializeField] private Enemy _enemy;
+    [SerializeField] private int _enemyCount;


HolyMonkey · 2021-04-22T15:58:00Z

+    public UnityAction StopGame;
+    [SerializeField] private Enemy _enemy;
+    [SerializeField] private int _enemyCount;
+    [SerializeField] private float _spawmRaius;


повнимательней

HolyMonkey · 2021-04-22T15:58:29Z

+
+public class EnemySpawner : MonoBehaviour
+{
+    public UnityAction StopGame;


Как спавнер может знать остановить игру или нет? Это не его ответственность. Т.е название во-первых неправильное со смысловой точки зрения а во-вторых вне нашего стиля

Спаунер должен только спаунить? А отслеживать смерти должен другой объект?

HolyMonkey · 2021-04-22T15:58:47Z

+            var enemy = Instantiate(_enemy, enemyPosition, Quaternion.identity,transform);
+            enemy.GetComponent<Enemy>().DeadEnemy += OnEnemyDie;
+        }


https://trello.com/c/orS9j3Rj/6-%D0%B4%D0%B8%D0%BD%D0%B0%D0%BC%D0%B8%D1%87%D0%B5%D1%81%D0%BA%D0%BE%D0%B5-%D1%81%D0%BE%D0%B7%D0%B4%D0%B0%D0%BD%D0%B8%D0%B5-%D0%BE%D0%B1%D1%8A%D0%B5%D0%BA%D1%82%D0%BE%D0%B2

HolyMonkey · 2021-04-22T15:59:05Z

+        if (_enemyCount == 0)
+        {
+            StopGame?.Invoke();
+        }


Почему это тут?

Changes as recommended

Change some scripts. Enemy now is abstact class.

HolyMonkey · 2021-05-06T12:24:02Z

+    [SerializeField] private float _time;
+    protected override void TakeDamage(PlayerMove playerMove)


Отдели пустой строкой

Готово.

HolyMonkey · 2021-05-06T12:24:15Z

+    [SerializeField] private float _time;
+    protected override void TakeDamage(PlayerMove playerMove)


Приём тут TakeDamage?

Приём тут TakeDamage?

Согласен. Некорректное название. Удалил.

HolyMonkey · 2021-05-06T12:24:19Z

+using System.Collections.Generic;
+using UnityEngine;
+
+public class Booster : Enemy


Booster это не враг

Booster это не враг

Разделил их на 2 отдельного класса. Для того что бы не было дубля кода - вынес схему движения и поиск точки движения в отдельный скрипт.

HolyMonkey · 2021-05-06T12:24:27Z

+    [SerializeField] private float _time;
+    protected override void TakeDamage(PlayerMove playerMove)
+    {
+        playerMove.BoostSpeed(_time);


Заебись нанёс урон

Заебись нанёс урон

Удалил.

HolyMonkey · 2021-05-06T12:24:43Z

+
+public class DefaultEnemy : Enemy
+{
+    protected override void TakeDamage(PlayerMove playerMove) { }


А это зачем?

А это зачем?

Убрал. Этот метод наследовался от родительного класса. Поэтому нужно его было реализовать.

HolyMonkey · 2021-05-06T12:25:08Z

+    [SerializeField] private float _radius;
+    [SerializeField] private float _speed;
+    private Vector2 _target;
+    public event UnityAction<Enemy> Dieing;


https://www.softschools.com/difference/dieing_vs_dying/236/

https://www.softschools.com/difference/dieing_vs_dying/236/

Понял, если я не имею в виду машину которая служит для резки бумаги, то используем Dying.

HolyMonkey · 2021-05-06T12:26:33Z

+    [SerializeField] private List<Enemy> _enemies;
+    [SerializeField] private int _enemiesCount;
+    [SerializeField] private float _spawnRaius;
+    public UnityAction AllEnemiesDied;


Событие от полей следует отделить пустой строкой

Готово.

HolyMonkey · 2021-05-06T12:26:45Z

+    private void Start()
+    {
+        for (int i = 0; i < _enemiesCount; i++)
+        {
+            Vector2 enemyPosition = Random.insideUnitCircle * _spawnRaius;
+            var enemy = Instantiate(_enemies[Random.Range(0,_enemies.Count)], enemyPosition, Quaternion.identity,transform);
+            enemy.Dieing += OnEnemyDie;
+        }
+    }


Не надо спавнер выдумывать, его небыло изначально

Не надо спавнер выдумывать, его небыло изначально

Удалил. Хотел немного привнести свое так сказать)

HolyMonkey · 2021-05-23T19:01:57Z

+    {
+        if (collision.TryGetComponent(out PlayerMove playerMove))
+        {
+            playerMove.BoostSpeed(_time);


Просто Boost

Просто Boost

Скорректировал

HolyMonkey · 2021-05-23T19:48:05Z

+        }
+    }
+
+    private void ChangeTarget()


Change слишком общий глагол и может создать путаницу: меняется наша цель или мы вносим изменение в объект цели?

Можно прибегнуть к лайф-хаку: "Как скажем при разговоре так и напишем". При обсуждение этого функционала с заказчиком или внутри команды, как бы ты сказал что делает противник?

Например: "Противник выбирает следующую цель" и тогда мы получим метод PickTarget
Например: "Противник ищет следующую цель" и тогда мы получим метод FindTarget

Если ты говоришь "Противник меняет цель" то это ок в разговорной речи но прямой перенос в название будет не ок.

Стоит ли раскрывать детали?

Например: NextRandomTarget, PickRandomTarget

Учитывая приватность метода детали раскрыть можно, но учти что уточняя метод в название мы ограничиваем его.

ChangeTarget настолько общий, что там может происходить что угодно. Это очень абстрактный метод, Change буквально значит любое изменение связанное с целями. Если бы метод назывался просто Change означало, что поменяться может всё что угодно. И это можно и нужно делать в определённых ситуациях, но не здесь.

Например PickRandomTarget сразу берёт несколько обязательств на себя:

Он поменяет состояние объекта (глагол Pick)

Он утилитарный и его основная задача это стратегия выбора (Random)

Это всё произойдёт с сущностью "цель" (Target)

Я бы был удивлён если бы этот метод делал например включение эффектов или какие-нибудь другие вещи.

Это такое бюджетное ООП. В каком-то идеальном мире это было бы так:

class EnemyMovement { Targets _targets; SurfaceAgent _agent; Update() { if(_agent.MoveTo(_targets.Current)) _targets.Next(); } }

Если убрать объекты то получится типа такого:
Next() или NextTarget() #

Change слишком общий глагол и может создать путаницу: меняется наша цель или мы вносим изменение в объект цели?

Можно прибегнуть к лайф-хаку: "Как скажем при разговоре так и напишем". При обсуждение этого функционала с заказчиком или внутри команды, как бы ты сказал что делает противник?

Например: "Противник выбирает следующую цель" и тогда мы получим метод PickTarget
Например: "Противник ищет следующую цель" и тогда мы получим метод FindTarget

Если ты говоришь "Противник меняет цель" то это ок в разговорной речи но прямой перенос в название будет не ок.

Стоит ли раскрывать детали?

Например: NextRandomTarget, PickRandomTarget

Учитывая приватность метода детали раскрыть можно, но учти что уточняя метод в название мы ограничиваем его.

ChangeTarget настолько общий, что там может происходить что угодно. Это очень абстрактный метод, Change буквально значит любое изменение связанное с целями. Если бы метод назывался просто Change означало, что поменяться может всё что угодно. И это можно и нужно делать в определённых ситуациях, но не здесь.

Например PickRandomTarget сразу берёт несколько обязательств на себя:

Он поменяет состояние объекта (глагол Pick)

Он утилитарный и его основная задача это стратегия выбора (Random)

Это всё произойдёт с сущностью "цель" (Target)

Я бы был удивлён если бы этот метод делал например включение эффектов или какие-нибудь другие вещи.

Это такое бюджетное ООП. В каком-то идеальном мире это было бы так:

class EnemyMovement { Targets _targets; SurfaceAgent _agent; Update() { if(_agent.MoveTo(_targets.Current)) _targets.Next(); } }

Если убрать объекты то получится типа такого:
Next() или NextTarget() #

Change слишком общий глагол и может создать путаницу: меняется наша цель или мы вносим изменение в объект цели?

Можно прибегнуть к лайф-хаку: "Как скажем при разговоре так и напишем". При обсуждение этого функционала с заказчиком или внутри команды, как бы ты сказал что делает противник?

Например: "Противник выбирает следующую цель" и тогда мы получим метод PickTarget
Например: "Противник ищет следующую цель" и тогда мы получим метод FindTarget

Если ты говоришь "Противник меняет цель" то это ок в разговорной речи но прямой перенос в название будет не ок.

Стоит ли раскрывать детали?

Например: NextRandomTarget, PickRandomTarget

Учитывая приватность метода детали раскрыть можно, но учти что уточняя метод в название мы ограничиваем его.

ChangeTarget настолько общий, что там может происходить что угодно. Это очень абстрактный метод, Change буквально значит любое изменение связанное с целями. Если бы метод назывался просто Change означало, что поменяться может всё что угодно. И это можно и нужно делать в определённых ситуациях, но не здесь.

Например PickRandomTarget сразу берёт несколько обязательств на себя:

Он поменяет состояние объекта (глагол Pick)

Он утилитарный и его основная задача это стратегия выбора (Random)

Это всё произойдёт с сущностью "цель" (Target)

Я бы был удивлён если бы этот метод делал например включение эффектов или какие-нибудь другие вещи.

Это такое бюджетное ООП. В каком-то идеальном мире это было бы так:

class EnemyMovement { Targets _targets; SurfaceAgent _agent; Update() { if(_agent.MoveTo(_targets.Current)) _targets.Next(); } }

Если убрать объекты то получится типа такого:
Next() или NextTarget() #

Спасибо. На основании вышеизложенного решил переименовать метод на ChooseRandomDestinationPoint()

HolyMonkey · 2021-05-23T19:49:15Z

+        AddEnemiesInWatchList();
+    }
+
+    private void AddEnemiesInWatchList()


Можно это назвать не наблюдаемым списком а "Живыми Противниками"

Можно это назвать не наблюдаемым списком а "Живыми Противниками"

Изменил название на AddAliveEnemies

HolyMonkey · 2021-05-23T19:49:31Z

+        if (_enemies.Count == 0)
+        {
+            AllEnemiesDied();
+        }


Это конечно же не в методе отписки должно быть

Это конечно же не в методе отписки должно быть

Перенес в отдельный метод CheckAliveEnemies

HolyMonkey · 2021-05-29T17:04:57Z

+        }
+    }
+
+    private void AllEnemiesDied()


Это не название метода

HolyMonkey · 2021-05-29T17:05:28Z

+        AddAliveEnemies();
+    }
+
+    private void AddAliveEnemies()


Add логично намекает, что данные должны прийти в параметр.
Здесь скорей как раз-таки FindEnemies

NemesisWar added 2 commits April 15, 2021 19:41

Refactoring Code

33a895a

Styled the code for standards. Changed script names. Changed the content of the scripts.

Add RequireComponents

c06ee11

Add RequireComponents in scripts

HolyMonkey requested changes Apr 22, 2021

View reviewed changes

NemesisWar added 3 commits April 23, 2021 01:41

update

c41b083

Changes as recommended

update

e205f82

Change some scripts. Enemy now is abstact class.

update

3430adc

HolyMonkey requested changes May 6, 2021

View reviewed changes

some changes

881e374

HolyMonkey requested changes May 23, 2021

View reviewed changes

update

391de3e

HolyMonkey requested changes May 29, 2021

View reviewed changes

		[SerializeField] private float _time;
		protected override void TakeDamage(PlayerMove playerMove)

Conversation

NemesisWar commented Apr 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NemesisWar May 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

NemesisWar May 8, 2021 •

edited

Loading