Search Unity

  1. Megacity Metro Demo now available. Download now.
    Dismiss Notice
  2. Unity support for visionOS is now available. Learn more in our blog post.
    Dismiss Notice

is this Good Design? OOP Loose Couple Principle?

Discussion in 'Scripting' started by NikoBusiness, Jul 1, 2016.

  1. NikoBusiness

    NikoBusiness

    Joined:
    May 6, 2013
    Posts:
    289
    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 :

    Code (CSharp):
    1.  using UnityEngine;
    2. using System.Collections;
    3. public class Skill {
    4.      public int Duration { get; set; }
    5.      SkillType Type;
    6.      public enum SkillType
    7.      {
    8.          Unknown,
    9.          Shield,
    10.          Life,
    11.      }
    12.      //Constructor
    13.      public Skill(int _duration,SkillType _type)
    14.      {
    15.          Duration = _duration;
    16.          Type = _type;
    17.      }
    18.      public SkillType getType()
    19.      {
    20.         return Type;
    21.      }
    22. }
    then there is a coin prefab that has attached CoinSkill script on it
    Code (CSharp):
    1.  using UnityEngine;
    2. using System.Collections;
    3. public class CoinSkill : MonoBehaviour {
    4.      Skill ShieldSkill = new Skill(10, Skill.SkillType.Shield);
    5.      GameCoreManager gameManager;
    6.      // Use this for initialization
    7.      void Start () {
    8.          Destroy(gameObject, ShieldSkill.Duration);
    9.          gameManager = GameObject.FindWithTag("GameController").GetComponent<GameCoreManager>();
    10.    
    11.      }
    12.    
    13.      void OnCollision2D(Collision2D collider)
    14.      {
    15.          if(collider.gameObject.tag == "Player")
    16.          {
    17.              gameManager.CollectCoin(ShieldSkill.getType(),collider.gameObject);
    18.              Destroy(gameObject);
    19.          }
    20.      }
    21. }
    & GameCoreManager script attached to GameManager Object
    Code (CSharp):
    1. using UnityEngine;
    2. using System.Collections;
    3. public class GameCoreManager : MonoBehaviour {
    4.      public void CollectCoin(Skill.SkillType _type,GameObject player)
    5.      {
    6.          if(_type == Skill.SkillType.Shield)
    7.          {
    8.              player.GetComponent<SkillActivator>().ActivateSkill(_type);
    9.          }
    10.      }
    11. }
    the skillactivator script attached to player (ball) object

    Code (CSharp):
    1. using UnityEngine;
    2. using System.Collections;
    3. public class SkillActivator : MonoBehaviour {
    4.      // Use this for initialization
    5.      void Start () {
    6.    
    7.      }
    8.    
    9.      // Update is called once per frame
    10.      void Update () {
    11.    
    12.      }
    13.      public void ActivateSkill(Skill.SkillType _type)
    14.      {
    15.          if(_type == Skill.SkillType.Shield)
    16.          {
    17.              activateShieldSkill();
    18.          }
    19.      }
    20.      public void activateShieldSkill()
    21.      {
    22.          //todo
    23.      }
    24. }
    so this is my design and something feels wrong anyone can suggest me a better design?
     
  2. makeshiftwings

    makeshiftwings

    Joined:
    May 28, 2011
    Posts:
    3,350
    It's hard to say without knowing what other things you'll eventually be adding to all of these classes, but at the moment:

    - You don't need the GameManager at all, at least not in this particular interaction. CoinSkill OnCollision2D already knows you hit the player and it knows that it's a Shield type, so you might as well just call collider.GetComponent<SkillActivator>().ActivateSkill(SkillType.Shield) from there.

    - The Skill class might be better off as an interface, like ISkill, that has a Duration and Type. That way you avoid possible issues in the future of trying to make complex skills that should inherit from a different base class. You could just make CoinSkill implement the ISkill interface.
     
  3. NikoBusiness

    NikoBusiness

    Joined:
    May 6, 2013
    Posts:
    289
    hey man thanks for the answer first of all!i already have implemented a very simpler method but i am trying to start programming with OOP design principles,the ISkill interface is a good idea man! thanks much simpler solution and i think it is a good design pattern too!im just trying to use all of the features a OOP language got because i dont want to get used to hardcoding because then its hard to unlearn!btw i check ur game, good job!
     
    makeshiftwings likes this.
  4. jimroberts

    jimroberts

    Joined:
    Sep 4, 2014
    Posts:
    560
    You should avoid using similarly named accessors for your fields. Your getType method should either be renamed or removed as it could cause confusion with the Object.GetType method. If you only want to allow read access to the SkillType you should use the readonly modifier because you set the skill type in the constructor.
    Code (csharp):
    1. public readonly SkillType SkillType;
    Actually.. why do you have a "SkillType" enum at all? The "SkillType" should be made explicit by derived classes. You can just use the Object.GetType() method.
    Code (csharp):
    1. public abstract class BaseSkill
    2. {
    3.    public int Duration { get; set; }
    4.  
    5.    public BaseSkill(int _duration)
    6.    {
    7.      Duration = _duration;
    8.    }
    9.  
    10.    //some other base implementations
    11. }
    12.  
    13. public class ShieldSkill : BaseSkill
    14. {
    15.    public ShieldSkill(int _duration) : base(_duration)
    16.    {
    17.    }
    18. }
    19.  
    20. public class LifeSkill : BaseSkill
    21. {
    22.    public LifeSkill(int _duration) : base(_duration)
    23.    {
    24.    }
    25. }
    I also agree with makeshiftwings.. When you implement behaviours the need for managers tends to disappear completely. Your "CoinSkill" class is actually not a skill. It's just a behaviour type so why not name it "CoinBehaviour"?