Gfrktyl

I'm making event system for my RPG game, and I want to know what I can do better, because I think that there are a lot things that can be better. Note that this code is still not completed but it works good, so could you tell me what of bad practises can you find there?

Event:

using UnityEngine;



[System.Serializable]

public class Event

{

    public enum Condition

    {

        VisitPlace,

        HaveItem,

        KillNpcs,

    }



    public enum Effect

    {

        ChangePlayerHP,

        GiveItemToPlayer,

        RemoveItemFromPlayerEquipment,

        GiveExperiencePoints,

        StartDialog,

        EndDialog,

        SpawnObject,

        TeleportPlayer,

        GiveQuest,

        RemoveQuest,

        IncrementActualStatusOfQuest,

        ThrowSpell,

        KillNpcs,

        EnableOrDisableAnotherEvent

    }



    public string    eventName;

    public Condition condition;

    public Effect    effect;



    public float            delay;                 //delay after which event have to be executed

    public int              intModifiter;          //int modifiter for every effect type that require it

    public float            floatModifiter;        //float modifiter for every effect type that require it

    public GameObject       gameObjectToSpawn;     //for spawnobject effect

    public CharacterStats condCharacterStats;    //character stats for condition

    public CharacterStats effCharacterStats;     //character stats for effect

    public int            conditionItemsIndexes; //indexes of items that will be required to play event

    public int            effectItemsIndexes;    //indexes of items that have to be removed or given after executing event effect



    public bool enable;           //enable or disable another event?

    public bool positiveResult;   //defines that condition was satisfied or not.

    public bool finished;         //is event finished?

    public bool isPlaying;        //is event playing?

    public bool isEnabled = true; //defines event activity



#region property drawer



    //bool contains item expanded status

    public bool expanded;



    //condition items array

    public bool condExpanded;

    public int  condArrLength;



    //effect items array

    public bool effExpanded;

    public int  effArrLength;



    //character stats condition array

    public bool chcExpanded;

    public int  chcArrLength;



    //character stats condition array

    public bool cheExpanded;

    public int  cheArrLength;



#endregion property drawer



    public void PlayEvent(EventManager em)

    {

        IEventConditionable tmp = null;

        switch (effect)

        {

            case Effect.GiveExperiencePoints:          tmp = new ExperienceEvent(intModifiter);                                 break;

            case Effect.GiveItemToPlayer:              tmp = new ItemEvent(effectItemsIndexes, true);                           break;

            case Effect.RemoveItemFromPlayerEquipment: tmp = new ItemEvent(effectItemsIndexes, false);                          break;

            case Effect.GiveQuest:                     tmp = new QuestEvent(QuestEventType.GiveQuest, intModifiter);            break;

            case Effect.RemoveQuest:                   tmp = new QuestEvent(QuestEventType.RemoveQuest, intModifiter);          break;

            case Effect.IncrementActualStatusOfQuest:  tmp = new QuestEvent(QuestEventType.IncrementQuestStatus, intModifiter); break;

            case Effect.EnableOrDisableAnotherEvent:   tmp = new AnotherEventsRelationsEvent(intModifiter, enable, em);         break;

            default:                                   Debug.Log("Event cannot be recognized.");                                break;

        }



        if(tmp != null)

            tmp.Play();

    }

}



public interface IEventConditionable

{

    void Play();

}



public enum QuestEventType

{

    GiveQuest,

    RemoveQuest,

    IncrementQuestStatus,

}



public class AnotherEventsRelationsEvent : IEventConditionable

{

    private int eventIndex;

    private bool enable;

    private EventManager em;



    public void Play()

    {

        em.events[eventIndex].isEnabled = enable;

    }



    public AnotherEventsRelationsEvent(int eventIndex, bool enable, EventManager em)

    {

        this.eventIndex = eventIndex;

        this.enable = enable;

        this.em = em;

    }

}



public class QuestEvent : IEventConditionable

{

    private QuestEventType questEventType; 

    private int questNr;



    public void Play()

    {

        if (questNr < QuestManager.getInstance.GetAllQuestsLength)

        {

            switch(questEventType)

            {

                case QuestEventType.GiveQuest:

                    QuestManager.getInstance.AddQuest(questNr);

                    break;

                case QuestEventType.RemoveQuest:

                    QuestManager.getInstance.RemoveQuest(questNr);

                    break;

                case QuestEventType.IncrementQuestStatus:

                    QuestManager.getInstance.IncrementQuestStatus(questNr);

                    break;

            }

        }



        QuestManager.getInstance.ShowQuestUpdatedIcon();

    }



    public QuestEvent(QuestEventType questEventType, int questNr)

    {

        this.questEventType = questEventType;

        this.questNr = questNr;

    }

}



public class ExperienceEvent : IEventConditionable

{

    private int experience;



    public void Play()

    {

        PlayerStatsManager.getInstance.stats.experience += experience;

    }



    public ExperienceEvent(int experience)

    {

        this.experience = experience;

    }

}



public class ItemEvent : IEventConditionable

{

    private int itemIndexes;

    private bool give;



    public void Play()

    {

        foreach (int itemIndex in itemIndexes)

        {

            if (give)

                EquipmentContainer.getInstance.AddInternalItem(itemIndex);

            else

                EquipmentContainer.getInstance.RemoveItem(itemIndex);

        }

    }



    public ItemEvent(int itemIndexes, bool give)

    {

        this.itemIndexes = itemIndexes;

        this.give = give;

    }

}

There are many variables because I created property drawer for this class.
The event class describes all events in game, it can give quests for player, remove it, give items etc.

EventManager:

using UnityEngine;

using System.Collections;

using System.Collections.Generic;

using System.Linq;



public class EventManager : MonoBehaviour

{

    private bool        checking;                 //is CheckEvents coroutine running?

    public  float       eventCheckerDelay = 0.1f; //divide every CheckEvents calling

    public  List<Event> events;



    private IEnumerator CheckEvents()

    {

        checking = true;



        if (!AllEventsAreFinished())

            foreach (Event e in events)

                if (e.isEnabled && !e.isPlaying && !e.finished && e.positiveResult)

                    StartCoroutine(PlayEvent(e));



        yield return new WaitForSeconds(eventCheckerDelay); //execute coroutine after every eventCheckerDelay value



        checking = false;

        StopCoroutine(CheckEvents());

    }



    private IEnumerator PlayEvent(Event e)

    {

        e.isPlaying = true;



        yield return new WaitForSeconds(e.delay);

        e.PlayEvent(this);



        e.isPlaying = false;

        e.finished = true;

        StopCoroutine(PlayEvent(e));

    }





    private bool AllEventsAreFinished()

    {

        return events.All(item => item.finished);

    }



    private void Update()

    {

        if(!checking)

               StartCoroutine(CheckEvents());

    }

}

The EventManager class manages all events and checks that they can be played.

EventHandler:

using UnityEngine;

using System.Collections;

using System.Linq;



public class EventHandler : MonoBehaviour {



    private bool         lastRes;

    private bool         collisionChecking;

    public  int        indexesOfEventsToEnable; //enable events with indexes assigned in array



    public  EventManager connectedEventManager;



    private IEnumerator CheckCollisions()

    {

        collisionChecking = true;

        bool playerIsColliding = Physics.OverlapBox(transform.position,

                                                    transform.localScale * 0.5f,

                                                    transform.rotation).

                                                    Where(item => item.gameObject.layer == LayerMask.NameToLayer("Player")).

                                                    ToArray().Length > 0; //Used overlapbox instead of onTriggerEnter or onCollisionEnter because overlap functions can catch many colliders in the same time



        if (playerIsColliding && !lastRes) //if player is colliding, and last sended result is false

            SendResult(true);

        else if (!playerIsColliding && lastRes)

            SendResult(false);



        yield return new WaitForSeconds(0.2f); //the script doesn't need huge precision, it can repeat execution after every 200 milliseconds

        collisionChecking = false;

        StopCoroutine(CheckCollisions());

    }



    public void SendResult(bool result)

    {

        foreach (int i in indexesOfEventsToEnable)

            if(connectedEventManager.events[i].isEnabled)

                connectedEventManager.events[i].positiveResult = result;



        lastRes = result; //set last result to sended as parameter result to stop send collision results

    }



    private void Update()

    {

        if (indexesOfEventsToEnable.Length > 0 && connectedEventManager && !collisionChecking)

            StartCoroutine(CheckCollisions());

        else

            Debug.LogError("Event manager or Event conditions are not assigned. Instance name: " + name);

    }

}

The EventHandler class that executes events when player collides with it.

public int intModifiter;          //int modifiter for every effect type that require it

public float floatModifiter;        //float modifiter for every effect type that require it

You write in the comment modifiter for every effect type so why not give the variable such a name: effectTypeModifier?

I nowhere see the floatModifiter being used. This could be removed.

public bool enable;           //enable or disable another event?

public bool isEnabled = true; //defines event activity

enable and isEnabled are too similar. You need a better name at least for one of them besides enable sound like an action so maybe canEnableNextEvent?

public bool positiveResult;   //defines that condition was satisfied or not.

How about success(ful)?

public bool finished;         //is event finished?

public bool isPlaying;        //is event playing?

The code could use some more consistency. Make them both have the is prefix or none of them.

public bool condExpanded;

public int condArrLength;

public bool effExpanded;

public int effArrLength;

..

You shouldn't use abbreviated names if they are not some common abbreviations like Html or Xml.

public interface IEventConditionable

{

  void Play();

}

This interface is named Conditionable but it contains a Play method. There is nothing about any conditions. The name IPlayable would be much better I think.

public void PlayEvent(EventManager em)

{

  IEventConditionable tmp = null;

  switch (effect)

  {

      case Effect.GiveExperiencePoints: tmp = new ExperienceEvent(intModifiter); break;

      case Effect.GiveItemToPlayer: tmp = new ItemEvent(effectItemsIndexes, true); break;

      case Effect.RemoveItemFromPlayerEquipment: tmp = new ItemEvent(effectItemsIndexes, false); break;

      case Effect.GiveQuest: tmp = new QuestEvent(QuestEventType.GiveQuest, intModifiter); break;

      case Effect.RemoveQuest: tmp = new QuestEvent(QuestEventType.RemoveQuest, intModifiter); break;

      case Effect.IncrementActualStatusOfQuest: tmp = new QuestEvent(QuestEventType.IncrementQuestStatus, intModifiter); break;

      case Effect.EnableOrDisableAnotherEvent: tmp = new AnotherEventsRelationsEvent(intModifiter, enable, em); break;

      default: Debug.Log("Event cannot be recognized."); break;

  }



  if (tmp != null)

      tmp.Play();

}

This method doesn't require the tmp variable. You don't store the result anywhere so you could simply call Play inside the switch just after you create an event:

switch (effect)

{

    case Effect.GiveExperiencePoints: new ExperienceEvent(intModifiter).Play(); break;

    ..

}

private bool lastRes;

And again an abbrevaited name. Does Res stand for result, resolution, rest or restaurant? You won't know this in a few days/weeks anymore.

bool playerIsColliding = Physics.OverlapBox(

    transform.position,

    transform.localScale * 0.5f,

    transform.rotation

)

.Where(item => item.gameObject.layer == LayerMask.NameToLayer("Player"))

.ToArray()

.Length > 0;

You can call simply Count on this.

bool playerIsColliding = Physics.OverlapBox(

    ..

)

.Where(item => item.gameObject.layer == LayerMask.NameToLayer("Player"))

.Count() > 0;

Wauw, many many things to go over.

These are my versions of your "Events"

public class GiveEquipment

{

    private int items;

    private Equipment equipment;



    public GiveEquipment(int items, Equipment equipment)

    {

        this.items = items;

        this.equipment = equipment;

    }



    public void Execute()

    {

        equipment.AddAll(items);

    }

}



public class RemoveEquipment

{

    private int items;

    private Equipment equipment;



    public RemoveEquipment(int items, Equipment equipment)

    {

        this.items = items;

        this.equipment = equipment;

    }



    public void Execute()

    {

        equipment.RemoveAll(items);

    }

}



public class GainExperience

{

    private int experience;

    private PlayerStats playerStats;



    public void Execute()

    {

        playerStats.experience += experience; // Unsafe if threaded

    }



    public GainExperience(int experience, PlayerStats playerStats)

    {

        this.experience = experience;

        this.playerStats = playerStats;

    }

}



public class GiveQuest

{

    private int questNr;

    private Quests quests;



    public GiveQuest(int questNr, Quests quests)

    {

        this.questNr = questNr;

        this.quests = quests;

    }



    public void Execute()

    {

        quests.Add(questNr);

        quests.ShowQuestUpdatedIcon();

    }

}



public class RemoveQuest

{

    private int questNr;

    private Quests quests;



    public RemoveQuest(int questNr, Quests quests)

    {

        this.questNr = questNr;

        this.quests = quests;

    }



    public void Execute()

    {

        quests.Remove(questNr);

        quests.ShowQuestUpdatedIcon();

    }

}



public class IncrementQuestStatus

{

    private int questNr;

    private Quests quests;



    public IncrementQuestStatus(int questNr, Quests quests)

    {

        this.questNr = questNr;

        this.quests = quests;

    }



    public void Execute()

    {

        quests.IncrementStatus(questNr);

        quests.ShowQuestUpdatedIcon();

    }

}

If you insist on your current structure for the class Event then you could redo PlayEvent like this:

    public void PlayEvent(EventManager em)

    {

        switch (effect)

        {

            case Effect.GiveExperiencePoints:

                new GainExperience(intModifiter, playerStats).Execute();

                break;

            case Effect.GiveItemToPlayer:

                new GiveEquipment(effectItemsIndexes, equipment).Execute();

                break;

            case Effect.RemoveItemFromPlayerEquipment:

                new RemoveEquipment(effectItemsIndexes, equipment).Execute();

                break;

            case Effect.GiveQuest:

                new GiveQuest(intModifiter, quests).Execute();

                break;

            case Effect.RemoveQuest:

                new RemoveQuest(intModifiter, quests).Execute();

                break;

            case Effect.IncrementActualStatusOfQuest:

                new IncrementQuestStatus(intModifiter, quests).Execute();

                break;

            case Effect.EnableOrDisableAnotherEvent:

                new AnotherEventsRelationsEvent(intModifiter, enable, em).Play();

                break;

            default:

                throw new UnknownEvent(effect);

        }

    }

There are many many things to dig into and many things could be improved with this setup.

1. The interface is unnecessary as you do not distribute these constructed events, so you do not need to communicate the exposed methods to anyone. If however you passed the actions to the Event type then an interface would make a lot of sense.




2. Explicit dependency injection. You see that I have removed all your manager.getInstance methods. It is bad bad practice to use singletons in any sense. And in no case is it a necessity. (though it feels easier when you do it). Explicit dependency injection is passing the variables as arguments when you construct the types.

Unity uses MonoBehaviours where you do not control the constructors, but you could construct a dependency system where you inject the types yourself. A bit much to go into, but you can have a think about it.

3. 
   Naming. Naming is hard but should be telling you something about the behaviour or content of the types inside of this. Manager, container, holder, EventConditional and the like, tells nothing. They can be managers, but don't name them as such. EquipmentContainer => Equipment, QuestManager => Quests, PlayerStatsManager => PlayerStats, etc. You get the point.


4. 
   Simplicity. Your events should be as simple as possible. I split your GiveQuest, RemoveQuest, IncrementQuestStatus into their own Event types, and removed your enum. This will give a good decoupling and their behaviour does not necessarily depend on each other. The code becomes a whole lot easier to read, which is the point. Simple code has fewer bugs than complex code. So keep it simple stupid (KISS from the U.S navy).


5. 
   Maintainability. (Then we have covered 3 out of 4 of the Code Health points). If you wanted to add a fourth version of Quest modification. In your old code you would have to modify the QuestEvent type. In this new code you never modify existing behaviours, you create a new type for the QuestEvent, therefore do not introduce new bugs into code that used to work. You decouple code to encapsulate problems to places you can easily fix.

I have many more things that I could go into, but I will not overwhelm you more than I already have. The overall code looks good, but these are just my points. Simplifying code often ensure much better quality in the long run.