11
\$\begingroup\$

I am working on my first object-oriented programming project, written in C# for Unity. My code does what I want it to do, but I am wondering if there is a better way to do it. I think I am building something that will be hard to manage going forward.

Info

I have a set of semi-related manager classes that are responsible for handling different scroll lists based on standardized templates that look like this:

Contains items:

items

Does not contain items:

no items

I currently have an AlertListManager, a MenuListManager, and an InventoryListManager. The different list managers are similar and so share some methods that perform the same behavior, like adding items to the list. However, not all lists are meant to have all of the behaviors, like changing the color of the list items. Additionally, some of the code inside the methods needs to be written differently between classes to handle different list item object types and unique list properties.

The class behaviors ultimately look like this:

behaviors

Code

Embedded is the method code for each list manager class. There are references to different screen class properties that are essentially the same between those classes when used in list management, so I have not included their code. Likewise for the individual list item classes. I split out a method to handle recycling Unity hierarchy GameObjects into a new class in case I needed to add to it later, so I added that too.

AlertListManager

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

public class AlertListManager : MonoBehaviour {

    [Header("Screens")]
    public HUDScreen hudScreen;
    public AlertScreen alertScreen;

    [Header("Prefabs")]
    //instantiate prefab for new alerts
    public GameObject alertItemPrefab;

    [Header("Alert Color Management")]
    public Color colorNormal1 = new Color32(255, 255, 255, 255);
    public Color colorHighlighted1 = new Color32(245, 245, 245, 255);
    public Color colorPressed1 = new Color32(200, 200, 200, 255);
    public Color colorNormal2 = new Color32(213, 213, 213, 255);
    public Color colorHighlighted2 = new Color32(203, 203, 203, 255);
    public Color colorPressed2 = new Color32(158, 158, 158, 255);

    private RecyclableObject ro = new RecyclableObject();

    void Awake()
    {
        UpdateAlertStatus();
    }

    private void UpdateAlertColors()
    {
        var layoutGroup = alertScreen.getLayoutGroup();
        var activeCountIndex = 0;

        foreach (Transform child in layoutGroup.transform)
        {
            // check active items only - recycle hierarchy entities
            if (child.gameObject.activeSelf)
            {
                if(activeCountIndex % 2 == 0)
                {
                    child.gameObject.GetComponent<AlertItem>().UpdateColors(colorNormal1, colorHighlighted1, colorPressed1);
                }
                else
                {
                    child.gameObject.GetComponent<AlertItem>().UpdateColors(colorNormal2, colorHighlighted2, colorPressed2);
                }

                activeCountIndex++;
            }

        }

    }

    private void UpdateAlertVisibility()
    {
        var count = alertScreen.GetAlertListCount();

        //toggle screen elements if returned count is 0
        alertScreen.ToggleAlertListDisplay(count);
        hudScreen.ToggleAlertMenuButtonDisplay(count);
    }

    public void UpdateAlertStatus()
    {
        UpdateAlertVisibility();
        UpdateAlertColors();
    }

    public void CloseAlert(AlertItem alertItem)
    {
        alertItem.Close();
        UpdateAlertStatus();
    }

    private GameObject TryToRecycleAlertGameObject()
    {
        var layoutGroup = alertScreen.getLayoutGroup();
        return ro.TryToRecycleGameObject(layoutGroup);
    }

    private void SetAlertInformation(GameObject go, AlertItem.AlertType alertType)
    {
        AlertItem ai = go.GetComponent<AlertItem>();

        //define alert type
        ai.UpdateAlertType(alertType);

        switch(alertType)
        {
            //add specific properties
        }

    }

    public void AddAlert(AlertItem.AlertType alertType)
    {
        var newAlert = TryToRecycleAlertGameObject();

        //if a deactivated child object exists, reactivate and move it
        if (newAlert != null)
        {
            SetAlertInformation(newAlert, alertType);
            //reactivate alert gameobject from component
            newAlert.GetComponent<AlertItem>().Open();
            //set alert position in screen
            alertScreen.RecycleAlertInList(newAlert);
        }
        //if not, create a new instance
        else
        {
            newAlert = (GameObject)Instantiate(alertItemPrefab);

            SetAlertInformation(newAlert, alertType);
            alertScreen.AddAlertToList(newAlert);
        }

        UpdateAlertStatus();
    }

}

MenuListManager

using UnityEngine;
using System.Collections;

public class MenuListManager : MonoBehaviour {

    [Header("Screens")]
    public MenuScreen menuScreen;

    [Header("Menu Color Management")]
    public Color colorNormal1 = new Color32(255, 255, 255, 255);
    public Color colorHighlighted1 = new Color32(245, 245, 245, 255);
    public Color colorPressed1 = new Color32(200, 200, 200, 255);
    public Color colorNormal2 = new Color32(213, 213, 213, 255);
    public Color colorHighlighted2 = new Color32(203, 203, 203, 255);
    public Color colorPressed2 = new Color32(158, 158, 158, 255);

    void Awake()
    {
        UpdateMenuStatus();
    }

    private void UpdateMenuColors()
    {
        var layoutGroup = menuScreen.getLayoutGroup();
        var activeCountIndex = 0;

        foreach (Transform child in layoutGroup.transform)
        {
            // check active items only - recycle hierarchy entities
            if (child.gameObject.activeSelf)
            {
                if(activeCountIndex % 2 == 0)
                {
                    child.gameObject.GetComponent<MenuItem>().UpdateColors(colorNormal1, colorHighlighted1, colorPressed1);
                }
                else
                {
                    child.gameObject.GetComponent<MenuItem>().UpdateColors(colorNormal2, colorHighlighted2, colorPressed2);
                }

                activeCountIndex++;
            }

        }

    }

    public void AddMenuItem()
    {
        //always create a new instance, no inactive GameObject will ever exist
        newMenuItem = (GameObject)Instantiate(menuItemPrefab);
        menuScreen.AddMenuItemToList(newMenuItem);

        UpdateMenuStatus();
    }


    public void UpdateMenuStatus()
    {
        UpdateMenuColors();
    }

}

InventoryListManager

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

public class InventoryListManager : MonoBehaviour {

    //public variables
    [Header("Screens")]
    public InventoryScreen invScreen;

    [Header("Prefabs")]
    //instantiate prefab for new gameobjects
    public GameObject invItemPrefab;

    private RecyclableObject ro = new RecyclableObject();

    void Awake()
    {
        UpdateInvStatus();
    }

    private void UpdateInvVisibility()
    {
        var count = invScreen.GetInvListCount();

        invScreen.ToggleInvListDisplay(count);
    }

    public void UpdateInvStatus()
    {
        UpdateInvVisibility();
    }

    private GameObject TryToRecycleInvGameObject()
    {
        var layoutGroup = invScreen.getLayoutGroup();
        return ro.TryToRecycleGameObject(layoutGroup);
    }

    private void SetInvItemInformation(GameObject go, LootItem li)
    {
        InventoryItem inv = go.GetComponent<InventoryItem>();

        //faster to access by field for large lists
        inv.UpdateNameText(li.itemName);
        inv.UpdateTypeText(li.itemType.ToString());

    }

    public void AddInvItem(LootItem lootItem)
    {
        var newInvItem = TryToRecycleInvGameObject();

        //if a deactivated child object exists, reactivate and move it
        if (newInvItem != null)
        {
            SetInvItemInformation(newInvItem, lootItem);
            //reactivate alert gameobject from component
            newInvItem.GetComponent<AlertItem>().Open();
            //set alert position in screen
            invScreen.RecycleInvItemInList(newInvItem);
        }
        //if not, create a new instance
        else
        {
            newInvItem = (GameObject)Instantiate(invItemPrefab);

            SetInvItemInformation(newInvItem, lootItem);
            invScreen.AddInvItemToList(newInvItem);
        }

        UpdateInvStatus();
    }

}

RecyclableObject

using UnityEngine;
using System.Collections;

public class RecyclableObject {

    public GameObject TryToRecycleGameObject(GameObject layoutGroup)
    {
        GameObject go = null;

        //look for a first deactivated child object
        foreach (Transform child in layoutGroup.transform)
        {
            // check active items only - recycle hierarchy entities
            if (!child.gameObject.activeSelf)
            {
                go = child.gameObject;
                break;
            }
        }

        return go;
    }

}

Thoughts

  • I am not sure whether to inherit from or to use composition with a base ListManager class. My understanding of inheritence is that all methods would be available in all classes -- but certain list manager classes should not have or need certain methods. But with composition, I don't think I can handle some of the special behavior, like with xxUpdateStatus() calling different combinations of xxUpdateVisibility(), xxUpdateColors(), and any potential future methods. Or maybe more importantly, xxAddItem() requiring different object types to be passed in.

  • I am already getting confused with the names of the methods. Those should probably change. But if composition is the best choice here, I don't know if it is a good practice to call them the same names as the methods in the instanced class. I would imagine so though.

  • A lot of the variables are public and I know it is typically bad to do that in C#. Unfortunately, it seems that Unity only allows you to work with public variables in the editor. There are ways to work around this, like what is detailed in http://schemingdeveloper.com/2014/11/07/c-properties-unity/ , but I don't think I want to go quite that far yet. I will leave them alone for now.

I have been trying to untangle this for a few days, so any help/review is extremely appreciated.

\$\endgroup\$
  • \$\begingroup\$ Obligatory link to Coding Horror: I Shall Call it... SomethingManager. Nice job on your first question :) \$\endgroup\$ – RobH Feb 3 '17 at 11:25
  • \$\begingroup\$ My only thought about this is to try generics.. \$\endgroup\$ – Richard Barker May 6 '17 at 18:22
1
\$\begingroup\$

Here is my thoughts:

Cut out 'Manager' from the name of all those managers. AlertListManager becomes AlertList. The job of an object is to manage its state and behaviour, so you don't need to explicitly say it. The same way when you create a new list, you don't call it ListManager, you simply say List.

When you're inside a class like AlertListManager you don't need to mention the class name over and over. UpdateAlertColors() becomes UpdateColors(), AddAlert() becomes Add(). It's pretty clear who you're referring to, the class itself. The same way when you have a list of integers you don't say aList.AddInteger(). You simply say aList.Add()

Let's have a look at this code:

[Header("Alert Color Management")]
public Color colorNormal1 = new Color32(255, 255, 255, 255);
public Color colorHighlighted1 = new Color32(245, 245, 245, 255);
public Color colorPressed1 = new Color32(200, 200, 200, 255);
public Color colorNormal2 = new Color32(213, 213, 213, 255);
public Color colorHighlighted2 = new Color32(203, 203, 203, 255);
public Color colorPressed2 = new Color32(158, 158, 158, 255);

I bet you copy-pasted it for the second time you wanted to write it. Well, this is good indication you need to factor this out and put it somewhere where other classes can access it. I normally make a Singleton class for a situation like this:

using UnityEngine;

public class ColorReferences : MonoBehaviour
{
    public static ColorReferences instanse;

    void Awake ()
    {
        instanse = this;
    }

    [Header("Alert Color Management")]
    public Color normal1 = new Color32(255, 255, 255, 255);
    public Color highlighted1 = new Color32(245, 245, 245, 255);
    public Color pressed1 = new Color32(200, 200, 200, 255);
    public Color normal2 = new Color32(213, 213, 213, 255);
    public Color highlighted2 = new Color32(203, 203, 203, 255);
    public Color pressed2 = new Color32(158, 158, 158, 255);

}

Then I can call it from anywhere like this ColorReferences.instanse.normal1;. Now I eliminated the duplication. Again, I'm not including Color prefix to any of the names inside the class, because it is pretty obvious that a class called ColorReferences surprising enough, contains references to colors.

Another thing which can be a personal style but worth mentioning is that place your methods according to how you call them. Something like this:

void A ()
{
    B ();
    C ();
}

void B ()
{
    B2 ();
    B3 ();
}

void B2 ()
{
    //code here...
}

void B3 ()
{
    //code here...
}

void C ()
{
    //code here...
}

This way you can read the code easily. So, I suggest to move your UpdateAlertStatus() method and put it below Awake() method.

Also in unity you don't have to make a member variable public to see it in the inspector editor. You can simply put [SerializeField] before any variable and it shows up in the inspector.

In terms of Inheritance or Composition, use none of them in this case. Don't make your code more complicated than it should be. Your list managers don't have any client, so no need to use any of those polymorphic technique here. You might say, how about code reuse or maintenance. Your code is not big enough to worry about those right now. Always go for simplicity. Sometimes, simplicity means using inheritance and composition, but that's a big topic and there are also many opposite opinions about it.

Just a quick side note. Unity3D has a component-based architecture. Try to have that in mind when designing your app architecture.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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