Spell and cooldown system for an Unity game

Question

So I have doubts about my code and if it is any good. I have a parent class Spell:

using UnityEngine;
using System.Collections;

public class Spell : MonoBehaviour {

    public enum SpellType { AttackType, AreaType, BuffType, DebuffType };

    public float damage;
    public float speed;
    public float cooldowntime;
    public SpellType type;

    public Spell()
    {

    }

    void Start () {

    }

    protected void Update () {
        if(type == SpellType.AttackType)
        {
            updateSpell();
        }
    }

    protected virtual void updateSpell()
    {

    }

    protected void deleteSpell()
    {
        Destroy(gameObject);
    }

    void OnTriggerEnter2D(Collider2D other)
    {
        if(other.tag == "Monster")
        {
            Interface.IMonster inter = other.GetComponent<Interface.IMonster>();
            inter.takeDamage(damage);
            deleteSpell();
        }
    }
}

Then one of the child classes:

using UnityEngine;
using System.Collections;

class WaterBolt : Spell
{
    private Vector3 mouseposition;
    private Vector3 screenpoint;
    private Vector2 offset;
    private float angle;

    void Start()
    {
        mouseposition = Input.mousePosition;
        screenpoint = Camera.main.WorldToScreenPoint(transform.localPosition);
        offset = new Vector2(mouseposition.x - screenpoint.x, mouseposition.y - screenpoint.y);
        angle = Mathf.Atan2(offset.y, offset.x) * Mathf.Rad2Deg;
        transform.rotation = Quaternion.Euler(0, 0, angle);

        Invoke("deleteSpell", 5f);
    }

    protected override void updateSpell()
    {
        transform.position += transform.right * speed * Time.deltaTime;
    }

}

Then this is what pretty much displays and drives the spell system with cooldown. Its a bit long and I would like to shorten but I can't think of something that would help. using UnityEngine; using System.Collections;

using UnityEngine.UI;
using System.Collections.Generic;

public class GuiSpellHandler : MonoBehaviour {

    public GameObject player;
    public int activenumber = 0;
    public GameObject spell1, spell2, spell3, spell4, spell5, spell6, spell7, spell8;
    public float spellcooldown1, spellcooldown2, spellcooldown3, spellcooldown4, spellcooldown5, 
                spellcooldown6, spellcooldown7, spellcooldown8;
    float spellcooldownnumber1, spellcooldownnumber2, spellcooldownnumber3, spellcooldownnumber4, spellcooldownnumber5,
            spellcooldownnumber6, spellcooldownnumber7, spellcooldownnumber8;
    public Image spell1cooldownimage, spell2cooldownimage, spell3cooldownimage, spell4cooldownimage, spell5cooldownimage,
                spell6cooldownimage, spell7cooldownimage, spell8cooldownimage;

    void Start () {
        player = GameObject.FindWithTag("Player");
    }

    void Update () {
        scrollSpells();
        displayCooldowns();
    }

    public void activateSpell(Transform trans)
    {
        switch (activenumber)
        {
            case 0:
                if (spellcooldown1 == 0 || spellcooldown1 < Time.time)
                {
                    Instantiate(spell1, trans.position, trans.rotation);
                    Spell spell = spell1.GetComponent<Spell>();
                    spellcooldown1 = Time.time + spell.cooldowntime;
                    spellcooldownnumber1 = spell.cooldowntime;
                }
                break;
            case 1:
                if (spellcooldown2 == 0 || spellcooldown2 < Time.time)
                {
                    Instantiate(spell2, trans.position, trans.rotation);
                    Spell spell = spell2.GetComponent<Spell>();
                    spellcooldown2 = Time.time + spell.cooldowntime;
                    spellcooldownnumber2 = spell.cooldowntime;
                }
                break;
            case 2:
                if (spellcooldown3 == 0 || spellcooldown3 < Time.time)
                {
                    Instantiate(spell3, trans.position, trans.rotation);
                    Spell spell = spell3.GetComponent<Spell>();
                    spellcooldown3 = Time.time + spell.cooldowntime;
                    spellcooldownnumber3 = spell.cooldowntime;
                }
                break;
            case 3:
                if (spellcooldown4 == 0 || spellcooldown4 < Time.time)
                {
                    Instantiate(spell4, trans.position, trans.rotation);
                    Spell spell = spell4.GetComponent<Spell>();
                    spellcooldown4 = Time.time + spell.cooldowntime;
                    spellcooldownnumber4 = spell.cooldowntime;
                }
                break;
            case 4:
                if (spellcooldown5 == 0 || spellcooldown5 < Time.time)
                {
                    Instantiate(spell5, trans.position, trans.rotation);
                    Spell spell = spell5.GetComponent<Spell>();
                    spellcooldown5 = Time.time + spell.cooldowntime;
                    spellcooldownnumber5 = spell.cooldowntime;
                }
                break;
            case 5:
                if (spellcooldown6 == 0 || spellcooldown6 < Time.time)
                {
                    Instantiate(spell6, trans.position, trans.rotation);
                    Spell spell = spell6.GetComponent<Spell>();
                    spellcooldown6 = Time.time + spell.cooldowntime;
                    spellcooldownnumber6 = spell.cooldowntime;
                }
                break;
            case 6:
                if (spellcooldown7 == 0 || spellcooldown7 < Time.time)
                {
                    Instantiate(spell7, trans.position, trans.rotation);
                    Spell spell = spell7.GetComponent<Spell>();
                    spellcooldown7 = Time.time + spell.cooldowntime;
                    spellcooldownnumber7 = spell.cooldowntime;
                }
                break;
            case 7:
                if (spellcooldown8 == 0 || spellcooldown8 < Time.time)
                {
                    Instantiate(spell8, trans.position, trans.rotation);
                    Spell spell = spell8.GetComponent<Spell>();
                    spellcooldown8 = Time.time + spell.cooldowntime;
                    spellcooldownnumber8 = spell.cooldowntime;
                }
                break;
            default:
                break;
        }
    }

    void scrollSpells()
    {
        if (Input.GetAxis("Mouse ScrollWheel") > 0f) // forward
        {
            if (activenumber == 7)
                activenumber = 0;
            else
                activenumber++;
        }
        else if (Input.GetAxis("Mouse ScrollWheel") < 0f) // backwards
        {
            if (activenumber == 0)
                activenumber = 7;
            else
                activenumber--;
        }
    }

    void displayCooldowns()
    {
        spell1cooldownimage.fillAmount = (spellcooldown1- Time.time) / spellcooldownnumber1;
        spell2cooldownimage.fillAmount = (spellcooldown2 - Time.time) / spellcooldownnumber2;
        spell3cooldownimage.fillAmount = (spellcooldown3 - Time.time) / spellcooldownnumber3;
        spell4cooldownimage.fillAmount = (spellcooldown4 - Time.time) / spellcooldownnumber4;
        spell5cooldownimage.fillAmount = (spellcooldown5 - Time.time) / spellcooldownnumber5;
        spell6cooldownimage.fillAmount = (spellcooldown6 - Time.time) / spellcooldownnumber6;
        spell7cooldownimage.fillAmount = (spellcooldown7 - Time.time) / spellcooldownnumber7;
        spell8cooldownimage.fillAmount = (spellcooldown8 - Time.time) / spellcooldownnumber8;      
    }
}

I hope someone could review this code for me and give me tips about how I use inheritance or how my SpellHandlerGUI can be improved!

Answer 1

You can get rid of a lot of repetition in activateSpell() by storing things in arrays and accessing them by the activeNumber. You can set up your arrays like so:

private GameObject[] spells;
private float[] spellCooldowns;
private float[] spellCooldownNumbers;

void Start () {
    player = GameObject.FindWithTag("Player");
    spells = new[] { spell1, spell2...
    spellCooldowns = new[] { spellcooldown1, spellcooldown2...
    spellCooldownNumbers = new[] { spellcooldownnumber1, spellcooldownnumber2...
}

You can then write activateSpell like this:

public void activateSpell(Transform trans)
{
    int i = activenumber;
    if (spellCooldowns[i] == 0 || spellCooldowns[i] < Time.time)
    {
        Instantiate(spells[i], trans.position, trans.rotation);
        Spell spell = spells[i].GetComponent<Spell>();
        spellCooldowns[i] = Time.time + spell.cooldowntime;
        spellCooldownNumber[i] = spell.cooldowntime;
    }
}

If you want, you can create a class to store a spell, its cooldown, and its cooldown number, so then you would only need to create one array.

You can use modulo arithmetic to make sure a number is within a certain range. scrollSpells() can be written like this:

void scrollSpells()
{
    if (Input.GetAxis("Mouse ScrollWheel") > 0f) // forward
    {
        activenumber = (activenumber + 1) % 7;
    }
    else if (Input.GetAxis("Mouse ScrollWheel") < 0f) // backwards
    {
        activenumber = (activenumber + 7 - 1) % 7;
    }
}

Instead of hard coding in 7 you can use the number of spells in the array.

void scrollSpells()
{
    if (Input.GetAxis("Mouse ScrollWheel") > 0f) // forward
    {
        activenumber = (activenumber + 1) % spells.Count();
    }
    else if (Input.GetAxis("Mouse ScrollWheel") < 0f) // backwards
    {
        activenumber = (activenumber + spells.Count() - 1) % spells.Count();
    }
}

Variable names should be lowerCamelCase, so activenumber should be activeNumber. Method names should be UpperCamelCase, so activateSpell() should be ActivateSpell().

Answer 2

1 . Your public fields in the Spell as far as I see aren't used,you declare a bunch of redundant variables when you already have such in your class.

2 . You can replace the switch case easily if your spell class know's a little bit more about what the actual spell is, if you don't want to expose too much data you might want to create a helper class, once you have this you can iterate over an array of Spell objects a class that has access to all the properties used in here :

            if (spellcooldown1 == 0 || spellcooldown1 < Time.time)
            {
                Instantiate(spell1, trans.position, trans.rotation);
                Spell spell = spell1.GetComponent<Spell>();
                spellcooldown1 = Time.time + spell.cooldowntime;
                spellcooldownnumber1 = spell.cooldowntime;
            }

3 . Your Spell class should probably have a boolean function which checks if the spell is off-cooldown so you can replace the if statements with the function call

public bool IsOnCooldown()
{
    return spellcooldown == 0 || spellcooldown < Time.time;
}

Another function maybe called CastSpell which might require some parameters like the gameObject you can even do this as an extension method so you dont always pass the spell that you need to cast, it should probably execute all this code :

                Instantiate(spell1, trans.position, trans.rotation);
                Spell spell = spell1.GetComponent<Spell>();
                spellcooldown1 = Time.time + spell.cooldowntime;
                spellcooldownnumber1 = spell.cooldowntime;

Like this you are restricting the access to your spell class which is one of the most important things in programming. If you now have the Spell array iterating over it would be trivial :

foreach(Spell spell in Spells)
{
    if(!spell.IsOnCooldown)
    {
        spell.CastSpell(...someParameters);
    }
}

Note You can add a function which checks if an appropriate keyboard key was pressed so you can trigger the spell this way.

4 . You might want to have something that allows you to check what cooldown is left on the spell like this you can easily work with the images/icons - a public property with a private setter or just a short function which returns an int you can decide what it will return maybe even the whole formula (spellcooldown1- Time.time) / spellcooldownnumber1 or just parts of it.

5 . After You've finished point #4 you can again declare an Image array, iterate over it with a for loop so you can combine it with the Spell array something like this :

for(int i = 0; i < Icons.Length; i++)
{
    Icons[i].fillAmount = Spells[i].GetCurrentCooldown();
}

This requires you to have your Spell[] sorted properly and also GetCurrentCooldown is the function I spoke of in point #4.

6 . Your scrollSpells method contains some magic numbers which are bad practice and should be avoided, aside from that you can use the ternary operator to shorten your lines there : activenumber = activenumber == 7 ? 0 : activenumber + 1;

7 . Your variable naming should look like this : spellCooldown or spell_cooldown instead of spellcooldown, function names should start with capital letters DisplayCooldowns rather than displayCooldowns.