patterncsharpMinor
Event system in Unity RPG game
Viewed 0 times
unitysystemgamerpgevent
Problem
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
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
Solution
public int intModifiter; //int modifiter for every effect type that require it
public float floatModifiter; //float modifiter for every effect type that require itYou 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 activityenable 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;Code Snippets
public int intModifiter; //int modifiter for every effect type that require it
public float floatModifiter; //float modifiter for every effect type that require itpublic bool enable; //enable or disable another event?
public bool isEnabled = true; //defines event activitypublic bool positiveResult; //defines that condition was satisfied or not.public bool finished; //is event finished?
public bool isPlaying; //is event playing?public bool condExpanded;
public int condArrLength;
public bool effExpanded;
public int effArrLength;
..Context
StackExchange Code Review Q#153257, answer score: 6
Revisions (0)
No revisions yet.