is this Good Design? OOP Loose Couple Principle?

hello! i want to show you if my Design is “correct” according to OOP principles.

info :

  • there is a ball which collects coins
  • the ball is the player.
  • there is a game manager

so before you see the Scripts i’ve made a graph :

i’ve made a Skill Class :

using UnityEngine;
using System.Collections;

public class Skill {

    public int Duration { get; set; }
    SkillType Type;



    public enum SkillType
    {
        Unknown,
        Shield,
        Life,
    }

    //Constructor
    public Skill(int _duration,SkillType _type)
    {
        Duration = _duration;
        Type = _type;
    }

    public SkillType getType()
    {
       return Type;
    }
}

then there is a coin prefab that has attached CoinSkill script on it

using UnityEngine;
using System.Collections;

public class CoinSkill : MonoBehaviour {

    Skill ShieldSkill = new Skill(10, Skill.SkillType.Shield);
    GameCoreManager gameManager;

	// Use this for initialization
	void Start () {
        Destroy(gameObject, ShieldSkill.Duration);
        gameManager = GameObject.FindWithTag("GameController").GetComponent<GameCoreManager>();
	
	}
	
    void OnCollision2D(Collision2D collider)
    {
        if(collider.gameObject.tag == "Player")
        {
            gameManager.CollectCoin(ShieldSkill.getType(),collider.gameObject);
            Destroy(gameObject);
        }
    }

}

& GameCoreManager script attached to GameManager Object

using UnityEngine;
using System.Collections;

public class GameCoreManager : MonoBehaviour {



    public void CollectCoin(Skill.SkillType _type,GameObject player)
    {
        if(_type == Skill.SkillType.Shield)
        {
            player.GetComponent<SkillActivator>().ActivateSkill(_type);
        }
    }

}

and finally the skillactivator script attached to player (ball) object

using UnityEngine;
using System.Collections;

public class SkillActivator : MonoBehaviour {

	// Use this for initialization
	void Start () {
	
	}
	
	// Update is called once per frame
	void Update () {
	
	}

    public void ActivateSkill(Skill.SkillType _type)
    {
        if(_type == Skill.SkillType.Shield)
        {
            activateShieldSkill();
        }
    }


    public void activateShieldSkill()
    {
        //todo
    }
}

I would probably encapsulate the skills more. Have the skill object store the method to be activated, to decouple it completely from the SkillActivator.

I however feel that the GameManager is an issue in its current state as it doesn’t do anything. You could just as easily call the SkillActivator class from the CoinSkill class. And if you refactor out the GameManager and follow my first advice of encapsulating the Skill logic in the Skill class, then the SkillActivator becomes redundant, and should be refactored away as well.

The bottom line is that you’re risking fragmenting rather than decoupling your code, if you just do it for the sake of “good design”.