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.

share|improve this question
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;
share|improve this answer
    
Thanks, I will correct that things. Could you tell me how do you think about quality of my code if I will correct it? – Michael Jan 22 at 15:20
    
@Michael we'll do, just keep in mind this: What should I do when someone answers my question? - you can post either a self-answer or a follow-up, don't edit your question ;-) – t3chb0t Jan 22 at 15:26
    
@Michael but I recommend you wait another day or two or three, you might receive more answers and more advices. – t3chb0t Jan 22 at 15:27
    
Ok, I didn't changed my code and I will not do it there. – Michael Jan 22 at 15:32

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.