Ревью проекта, кто сможет глянуть?

Question

[~KraGen~]

Здравствуйте форумчани, делал тестовое задание для работы на вакансию джуна, отклонили, ошибки толком не написали, кому не сложно гляньте и ткните носом в ошибки что бы исправлялся, буду очень признателен
Гит

Comments

[K0TlK]

Задание где?

[~KraGen~]

K0TlK, Вот

[K0TlK]

KraGen,

[SerializeField] private float movementSpeed;
    [SerializeField] private float jumpForce;

    [SerializeField] private GameObject playerModel;

    public bool play = false;
    
    private PlayerInput _playerInput;
    private Transform player;

Нет определенного код стайла. Где-то есть нижнее подчеркивание у private полей, где-то нет. Паблик поле с маленькой буквы. Существуют общепринятые нормы, следуй им, если нет каких-то других.
https://docs.microsoft.com/en-us/dotnet/csharp/fun...
https://github.com/dotnet/runtime/blob/main/docs/c...

[SerializeField] private GameObject startScreen;
    [SerializeField] private GameObject loseScreen;
    
    public delegate void UIEvents();
    public static event UIEvents TapOnScreen;

Взаимодействие с геймобжектами, а не с конкретными компонентами. Статика. Ненужный делегат, для всего того, что ты делаешь, существует Action.

public class CubeController
public static event CubeEvent ModifiedCubesCount
public static event CubeEvent CollectCube

Невнятный нейминг. С первого взгляда не понятно что эти ивенты из себя представляют. Что такое ModifiedCubesCount? Количество кубов? Почему тогда это ивент, а не свойство? Что такое CollectCube? Метод? Если нет, то почему как метод назван? Что он делает? Куб собирает? CubesCountChanged и CubeCollected их следовало назвать. Что такое контроллер? Что он контролирует? Название объекта должно показывать не то что он делает, а то кем он является. CubesStack или что-то типа этого.

if (other.gameObject.CompareTag("Cube") && !other.gameObject.GetComponent().collected && collected)

Тэги. Могу поспорить, что одной из главных причин отказа были тэги. Не надо их использовать. Вместо этого огромного условия можно было сделать одно - TryGetComponent(out Cube cube>. Всё.

var higherCube = holder.transform.GetChild(0)
holder.transform.childCount
holder.transform.GetChild(i).localPosition.y

Взаимодействие с чайлд элементами. Почему не сделать место, где все твои кубы будут храниться? Почему не отделить отдельный компонент Cube и взаимодействовать уже с ним, а не с примитивами?

_builder = FindObjectOfType()
Зачем использовать Find..., когда можно просто прокинуть зависимость через инспектор?

gameObject.transform.position = new Vector3(Mathf.Clamp(position.x + (_playerInput.axisHorizontal / 4) * Time.deltaTime,-2,2), position.y, position.z + movementSpeed * Time.deltaTime);

Ты сам то понимаешь что здесь происходит? Нечитабельно. Что за магические числа? Откуда 4, откуда -2, 2?

axisHorizontal = Mathf.Clamp(Input.mousePosition.x - startPos,-50,50);
Почему именно -50 и 50? Почему не от 0 до 1?

Весь инпут в одном файле. Не выделен интерфейс под инпут, чтобы его потом можно было подменить другим. В будущем этот класс разрастется до 2000 строк. По 1000 на каждый #if

[K0TlK]

KraGen, Ну и нет никакой архитектуры вообще. Все работает на монобехах.

[~KraGen~]

K0TlK, Благодарю что глянул. магические числа это да тупонул. Я когда делал думал тип "ну щас напишу а когда буду сдавать нужно будет в отдельные поля вынести" как итог забыл. На счёт кодстайла ну его толком не учил только просматривал. На счёт тегов - не знал. Буду знать. За Чайлд елементы да меня за них пнули понял что не нужно их постоянно получать лучше 1 раз закешировать. И на счёт ГетКомпонент тоже так же. А вот на счёт инпута то не особо понимаю зачем интерфейсы нужны если их прийдёться постоянно писать. Тип спрашивал у знакомого Мидла он сказал что с опытом поймёшь. А на счёт архитектуры то игра впринципе не сложная и я толком пока не понимаю какая может быть архитектура. Если не сложно приведи пример пожалуйста. И да ещё раз спасибо за ревью :)

[K0TlK]

KraGen, Интерфейсы - это контракт. Все, кто реализует интерфейс, обязуются соблюдать этот контракт. Когда ты выделяешь интерфейс, то у тебя все взаимодействие происходит через этот интерфейс. Ты не зависишь от какой-то конкретной реализации этого интерфейса. Пример. Есть интерфейс IWindow

public interface IWindow
{
    void Open();
    void Close();
}

И есть несколько его реализаций:

public class InventoryWindow : IWindow{}
public class HealthWindow : IWindow{}
public class HouseWindow : IWindow{}

Первый - ui окно инвентаря, второй - окно отображения хп, третий окно в каком-то доме на сцене.

И есть какой-то объект, который может эти окна открывать:

public class SomeObject
{
    public void OpenWindow(IWindow window)
    {
        window.Open();
    }
}

И этому объекту абсолютно не важно какое окно ему подсунут. Будь то ui или окно на сцене. Он не зависит от реализации окна, он зависит от абстракции окна.

зачем интерфейсы нужны если их прийдёться постоянно писать

Куда проще что-либо переделать/расширить, когда у тебя зависимости не от конкретной реализации, а от какой-то абстракции. Может показаться, что, если есть только одна реализация у интерфейса, то можно этот интерфейс и не писать вовсе, но, когда потребуется что-то изменить/расширить, то это сделать будет сложнее, если этого интерфейса у тебя не будет.

А на счёт архитектуры то игра впринципе не сложная и я толком пока не понимаю какая может быть архитектура

Всю игру можно сделать, поместив на пустую сцену всего один монобех, который будет являться точкой входа. И там будут инициализироваться все компоненты и передаваться все зависимости

[~KraGen~]

K0TlK, Понял, спасибо огромное :)

Answer 1

[freeExec]

UIManager.TapOnScreen += (() => play = true);
UIManager.TapOnScreen -= (() => play = true);

Это так не работает

playerModel.GetComponent<Rigidbody>().isKinematic = true;
playerModel.transform.DOMoveY(playerModel.transform.position.y + jumpForce, 0.1f);

Эээ, а нафига тогда физика?

private void Update()
{
   gameObject.transform.DOMoveZ(player.position.z - 13, speed);

Не, может дотвин и страхует от таких глупостей, а если нет?

Но в целом оформление кода лучше, что у большинства, спрашивающих на тостере.
Хотя от статических событий на контролёрах я бы посоветовала избавиться.

Comments

[~KraGen~]

Это так не работает

Ну, я подозревал это просто не хотел создавать метод который будет выполнять одну строчку кода.

Эээ, а нафига тогда физика?

ну тип мне нужно просто что бы игрок поднимался на 1еденицу вверх. А если делать через физику то при подборе большого количества кубов игрок улетает. Хотя можно было бы пошаманить с бубном и было бы норм и через физику. Это типа лёгкий путь был

Не, может дотвин и страхует от таких глупостей, а если нет?

Ну вот в это меня пнули. Лучше было бы через Лерп сделать да и всё думаю

Но в целом оформление кода лучше

Крутякю

Хотя от статических событий на контролёрах я бы посоветовала избавиться.

Понял :) Спасибо за ревью :)

[Eugene]

KraGen,

не хотел создавать метод который будет выполнять одну строчку кода

такие методы - абсолютная норма. Вы, наверное, любите, когда у вас 2000 строк в каждом методе, да?)

[~KraGen~]

Eugene, нет ну просто не хотел что бы была тупо одна строка в одном методе. Но раз это норма то окей