Написал систему бонусов для ранера в Unity. Насколько правильно я это сделал?

Для начинающего не плохо. Но ответственность каждого класса размыта.

• BonusPlayer, имеет данные о длительности, визуализации, сам себя собирает, активирует и деактивирует, пиццу заказывает, детей из детского сада забирает... многовато!
• BonusUI по идее занимается отображением, но сам вычисляет время и работает без конечно...
• Player... почему он вообще занимается UI?

---

Нужен объект трека который можно собирать:

// collectable item model 
public abstract class CollectableItem : MonoBehaviour
{
    public UnityEvent OnCollect;

    public void Collect ()
    {
        OnCollect?.Invoke();
    }
}

_soundEffect и _specialEffect не нужны, потому что их можно запустить через событие, и не только их, а все что угодно, хоть настройки материала поменять, хоть вообще без ничего. Подоранный предмет может быть чем угодно, 100gold, хоть 10xp, _duration и _icon тут тоже лишние.

---

То что отличает именно бафы.

public interface IBuff
{
    BuffData Data { get; }
    float elapsed { get; }
}

// collectable buff model 
public class CollectableBuff : CollectableItem
{
    [SerializeField] private BuffData _data;
    
    public BuffData Data => _data;

    public IBuff Buff { get; }
}

// набор данных связан, образуя вместе единицу и передается пакетом,
// как например в UI визуализацию, поэтому нет смысла их разделять
[Serializable]
public struct BuffData
{
    public string title;
    public Sprite icon;
    public float duration;
}

Наследники могут быть бонусами абсолютно любого типа.

// multiply all score income
public class ScoreMultiplayerBuff : CollectableBuff
{
    [SerializeField] private float _multilayer;

    public float Multiplayer => _multilayer;

    public IBuff Buff => new MultiplayerModifier(Data, _multilayer);
}

// add score to each monster kill
public class KillScoreBonusBuff : CollectableBuff
{
    [SerializeField] private float _amount;

    public float Amount => _amount;

    public IBuff Buff => new KillModifier(Data, _amount);
}

---

Сбор предметов это отдельная ответственность, ни как не связанная с Player чья задача бежать, прыгать и т.д.

// collect collectable items
public class Collector : MonoBehaviour
{
    [SerializeField] private BuffHandler _buff;
    [SerializeField] private Wallet _wallet;
    [SerializeField] private Inventory _inventory;
    
    private void OnTriggerEnter (Collider other)
    {
        if (other.TryGetComponent(out CollectableItem item))
        {
            if (item is CollectableBuff buff)
                OnCollect(buff);
            else if (item is CollectableCurrency currency)
                OnCollect(currency);
            else if (item is CollectableResources resources)
                OnCollect(resources);
            OnCollect(item);
        }
    }

    private void OnCollect (CollectableBuff buff)
    {
        _buff.Add(buff.Buff);
    }

    private void OnCollect (CollectableCurrency currency)
    {
        _wallet.Add(currency.type, currency.amount);
    }

    private void OnCollect (CollectableResources resources)
    {
        _inventory.Add(resources.content);
    }
}

---

У тебя бафф подбирал и обрабатывал сам себя, а BuffUI не имел с ним никакой связи. Должен быть обработчик бафов, занимающийся их жизненным циклом, а визуализация в интерфейсе и обработка эффекта уже головняк других классов, о которых обработчик и знать не знает.

// handles the life cycle of buffs
public class BuffHandler : MonoBehaviour
{
    // BuffView подписывается на эти события и занимается только отображением
    // Еще нужен обработчик который проверяет бафы на типы и отправляет кому нужно,
    // например, если IBuff это ScoreModifier (MultiplayerModifier, KillModifier),
    // то он передается ScoreHandler обрабатывающйи получение очков,
    // подобно Collector который разделяет суп и мух на более верхнем уровне CollectableItem
    public Action<IBuff> OnAdded;
    public Action<IBuff> OnExpired;
    private List<IBuff> _buffs;

    public void Start ()
    {
        _buffs = new List<IBuff>();
    }

    public void Add (IBuff buff)
    {
        _buffs.Add(buff);
        OnAdded?.Invoke(buff);
    }

    public void Expired (IBuff buff)
    {
        _buffs.Remove(buff);
        OnExpired?.Invoke(buff);
    }

    private void Update ()
    {
        // for с конца поскольку истекшие бафы удаляются
        for (int i = _buffs.Count-1; i > -1; i--)
        {
            IBuff buff = _buffs[i];
            buff.elapsed += Time.deltaTime;
            if (buff.elapsed >= buff.Data.duration)
                Expired(buff);
        }
    }
}

---

п.с. классы типа BuffHandler, ScoreHandler, Wallet, Inventory не обязательно должны быть MonoBehaviour.

---

Закрытости и наследование хромает

• делать protected F get/set свойство для private _f это тоже самое, что поставить самому полю _f модификатор доступа protected, атрибут [SerializeField] на него так-же распространяется.
• наследники BonusPlayer, могут делать с полем protected Player что вздумается, хотя по задумке не должны, свойство, позволяющее только protected get отсутствует и поле должно быть private, либо без поля и только свойство protected Player { get; private set; }.
• virtual M() метод плохая практика, из-за того, что в наследнике может не быть base.M(). Если базовое выполнение обязательно, то пускай M() в конце себя вызывает пустой метод virtual/abstract OnM() и у наследника не будет такого выбора, который ему не нужен.