Question Checking the type of an object Is this a good practice?

Hi, I am wondering about the quality of my code. I learned that checking object types is not a good solution.

To better illustrate the problem, I will show the code below

Spoiler: TravelUIManager.cs

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4. using UnityEngine.UI;
5.  
6. namespace DungeonManager
7. {
8.     public class TravelUIManager : MonoBehaviour
9.     {
10.         private UIManager uiMgr;
11.  
12.         //Travel ui elements
13.         [Header("Travel Ui Elements")]
14.         [SerializeField]
15.         private Text travelUI_cityName;
16.         [SerializeField]
17.         private Text travelUI_requiredLevel;
18.         [SerializeField]
19.         private Text travelUI_desc;
20.         [SerializeField]
21.         private GameObject travelUI;
22.  
23.         [Header("Expedition Ui elements")]
24.         //Expedition ui elements
25.         [SerializeField]
26.         private GameObject expeditionUI;
27.         [SerializeField]
28.         private Text expeditionUI_locationName;
29.         [SerializeField]
30.         private Text expeditionUI_requiredlevel;
31.         [SerializeField]
32.         private Text expeditionUI_chance;
33.         [SerializeField]
34.         private Text expeditionUI_time;
35.  
36.         public void init(UIManager uIManager)
37.         {
38.             this.uiMgr = uIManager;
39.         }
40.         public void EnableUI(Location location)
41.         {
42.             if(location is CityLocation)
43.             {
44.                 EnableTravelUI(location as CityLocation);
45.             }
46.             else if(location is ExpeditionLocation)
47.             {
48.                 EnableExpeditionUI(location as ExpeditionLocation);
49.             }
50.             else
51.             {
52.                 Debug.LogWarning("The given type is invalid");
53.             }
54.         }
55.  
56.         private void EnableExpeditionUI(ExpeditionLocation location)
57.         {
58.             LoadExpeditionUI(location);
59.             uiMgr.DisableMapUI();
60.             expeditionUI.SetActive(true);
61.             GameManager.Instance.camMovement.EnableMoving = false;
62.         }
63.         private void LoadExpeditionUI(ExpeditionLocation location)
64.         {
65.             expeditionUI_locationName.text = location.name;
66.             expeditionUI_requiredlevel.text = location.ReqLevel.ToString();
67.         }
68.         private void EnableTravelUI(CityLocation location)
69.         {
70.             LoadTravelUI(location);
71.  
72.             travelUI.SetActive(true);
73.             uiMgr.DisableMapUI();
74.             GameManager.Instance.camMovement.EnableMoving = false;
75.         }
76.         private void LoadTravelUI(CityLocation location)
77.         {
78.             travelUI_cityName.text = location.Name;
79.             travelUI_requiredLevel.text = location.ReqLevel.ToString();
80.             travelUI_desc.text = location.Desc;
81.         }
82.  
83.         public void DisableUI()
84.         {
85.             travelUI.SetActive(false);
86.             expeditionUI.SetActive(false);
87.         }
88.     }
89.  
90. }

In function EnableUI I check the type and display different ui depending on the type.
I will send 2 more related scripts

Spoiler: Location.cs

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4.  
5. namespace DungeonManager
6. {
7.     public abstract class Location : ScriptableObject
8.     {
9.         [SerializeField]
10.         private string locName;
11.         public string Name
12.         {
13.             get { return this.name; }
14.         }
15.         [SerializeField]
16.         private string description;
17.         public string Desc
18.         {
19.             get { return this.description; }
20.         }
21.         [SerializeField]
22.         private uint requiredLevel;
23.         public uint ReqLevel
24.         {
25.             get { return this.requiredLevel; }
26.         }
27.     }
28.     [CreateAssetMenu(fileName = "CityLocation", menuName = "CreateGameData/CityLocation", order = 1)]
29.     public class CityLocation : Location
30.     {
31.  
32.     }
33.     [CreateAssetMenu(fileName = "ExpeditionLocation", menuName = "CreateGameData/ExpeditionLocation", order = 1)]
34.     public class ExpeditionLocation : Location
35.     {
36.  
37.     }
38. }
39.  

Spoiler: LocationPinUI.cs

Code (CSharp):

1. using System.Collections.Generic;
2. using UnityEngine;
3. using UnityEngine.UI;
4. using UnityEngine.EventSystems;
5.  
6. namespace DungeonManager
7. {
8.     public class LocationPinUI : MonoBehaviour, IPointerClickHandler
9.     {
10.         [SerializeField]
11.         private Location location;
12.  
13.         [SerializeField]
14.         private Text nameText;
15.  
16.         public void Start()
17.         {
18.             //Setup hover panel
19.             nameText.text = location.Name;
20.         }
21.         public void OnPointerClick(PointerEventData eventData)
22.         {
23.             GameManager.Instance.UIMgr.travelUIManager.EnableUI(location);
24.         }
25.     }
26. }

This class is responsible for activating the OnEnable function depending on the location assigned to it.

So my question is:
Is it well done? if not how can it be solved better? and what about type checking in "OnEnable"
shouldn't I do that?

 

It's maybe not the best practice in the world, but for this simple case if it works, I wouldn't worry about it. On a side note, there's a little better syntax you can use around here:

Code (CSharp):

1.             if(location is CityLocation)
2.             {
3.                 EnableTravelUI(location as CityLocation);
4.             }

Try this instead:

Code (CSharp):

1.             if(location is CityLocation cityLocation)
2.             {
3.                 EnableTravelUI(cityLocation);
4.             }

 

Thanks for the opinion.

 

I suspect your are right and it is just a syntax difference, but I am not positive, since I think you are introducing a local variable, though I am not really familiar with that syntax myself. I have been using C# a long time and I am bad about sticking with what I know when there are improvements. And TBH, I prefer the first option; it's a little clearer to me.