well structured?

Are there any ideas that you can give me to make this code well structured? I feel like this is ok enough but I need your opinions to be sure and maybe learn some more from you.

Code (CSharp):

1. public class GameManager : MonoBehaviour
2. {
3.     #region vars
4.     public PlayerMove[] players;
5.     public GameObject[] deadPoints;
6.     public GameObject[] enterancePoints;
7.     public GameObject enterance;
8.     public GameObject target;
9.     public GameObject playButt;
10.     public Text timerTxt;
11.     public float time;
12.     public float resetTime;
13.     string tempTimer;
14.     public int nextSceneIndex;
15.     GameObject temp;
16.     int index=0;
17.     #endregion
18.     void Start()
19.     {
20.         players = FindObjectsOfType(typeof(PlayerMove)) as PlayerMove[];
21.         foreach(PlayerMove player in players)
22.         {
23.             player.GetComponentInChildren<MeshRenderer>().enabled = false;
24.             player.GetComponentInChildren<Collider>().enabled = false;
25.             player.GetComponent<PlayerMove>().canMove = false;
26.         }
27.         temp = players[index].gameObject;
28.         StartMove();
29.     }
30.  
31.     void Update()
32.     {
33.         tempTimer = string.Format("{0:00}", time);
34.         time -= 1 * Time.deltaTime;
35.         timerTxt.text = tempTimer;
36.         if (time <= 0)
37.         {
38.             ReTry();
39.         }
40.     }
41.     public void PlayGame() //Starts time and controls
42.     {
43.         playButt.SetActive(false);
44.         Time.timeScale = 1;
45.         temp.GetComponent<PlayerMove>().canMove = true;
46.     }
47.     public void Game()//When reache to target changes car/enterance/target
48.     {
49.         int tempLength = players.Length;
50.         temp.GetComponent<PlayerMove>().canMove = false;
51.         index+=1;
52.         Debug.Log(index);
53.  
54.         if(index<tempLength)
55.         temp = players[index].gameObject;
56.  
57.         StartMove();
58.     }
59.     void StartMove()//prepares scene to next step
60.     {
61.         int tempLength = players.Length;
62.         if (index == tempLength)
63.         {
64.             SceneManager.LoadScene(nextSceneIndex);
65.             index = 0;
66.         }
67.  
68.         if (enterancePoints[index] != null)
69.         {
70.             enterance.transform.position = enterancePoints[index].transform.position;
71.             enterance.transform.rotation = enterancePoints[index].transform.rotation;
72.         }
73.         temp.transform.position = enterance.transform.position;
74.         temp.transform.rotation = enterance.transform.rotation;
75.         temp.GetComponentInChildren<MeshRenderer>().enabled = true;
76.         temp.GetComponentInChildren<Collider>().enabled = true;
77.  
78.         if(deadPoints[index]!=null)
79.         target.transform.position = deadPoints[index].transform.position;
80.  
81.        
82.         ResetTimer();
83.         playButt.SetActive(true);
84.         Time.timeScale = 0;
85.        
86.     }
87.     public void ReTry()//when hits others
88.     {
89.         temp.transform.position = enterance.transform.position;
90.         temp.transform.rotation = enterance.transform.rotation;
91.         temp.GetComponent<PlayerMove>().canMove = false;
92.         ResetTimer();
93.         playButt.SetActive(true);
94.         Time.timeScale = 0;
95.     }
96.     void ResetTimer()
97.     {
98.         time = resetTime;
99.     }
100. }

---------------------------------------------------------------------------------------------------------------------------------

Code (CSharp):

1. public class TurningApplier : MonoBehaviour, IPointerDownHandler, IPointerUpHandler
2. {
3.     public PlayerMove[] players;
4.     public bool left;
5.     public bool right;
6.     private void Start()
7.     {
8.         players = FindObjectsOfType(typeof (PlayerMove)) as PlayerMove[];
9.      
10.     }
11.     public void OnPointerDown(PointerEventData eventData)//right and left turning control
12.     {
13.         if (right == true)
14.         {
15.             foreach (PlayerMove player in players)
16.             {
17.                 player.right = true;
18.             }
19.         }
20.         if (left == true)
21.         {
22.             foreach (PlayerMove player in players)
23.             {
24.                 player.left = true;
25.             }
26.         }
27.  
28.     }
29.     public void OnPointerUp(PointerEventData eventData)
30.     {
31.        
32.             foreach (PlayerMove player in players)
33.             {
34.                 player.right = false;
35.                 player.left = false;
36.             }
37.        
38.     }
39. }

---------------------------------------------------------------------------------------------------------------------

Code (CSharp):

1. public class PlayerMove : MonoBehaviour
2. {
3.     public bool right;
4.     public bool left;
5.     public float speed;
6.     public float rotationSpeed;
7.     public bool canMove;
8.     void Update()
9.     {
10.         if (canMove)
11.         {
12.             transform.position += transform.forward / speed;
13.  
14.             //// To test on PC
15.             if (Input.GetKey(KeyCode.RightArrow))
16.             {
17.                 transform.Rotate(Vector3.up * (rotationSpeed * Time.deltaTime));
18.             }
19.             if (Input.GetKey(KeyCode.LeftArrow))
20.             {
21.                 transform.Rotate(Vector3.down * (rotationSpeed * Time.deltaTime));
22.             }
23.             ////
24.             if (right)
25.             {
26.                 transform.Rotate(Vector3.up * (rotationSpeed * Time.deltaTime));
27.             }
28.             if (left)
29.             {
30.                 transform.Rotate(Vector3.down * (rotationSpeed * Time.deltaTime));
31.             }
32.         }
33.     }
34.     private void OnTriggerEnter(Collider other)
35.     {
36.         if (other.tag == "Player")
37.         {
38.             GameManager game = FindObjectOfType(typeof(GameManager)) as GameManager;
39.             game.ReTry();
40.         }
41.     }
42. }
43.  

-----------------------------------------------------------------------------------------------------------------------------

Code (CSharp):

1. public class TargetTrigger : MonoBehaviour
2. {
3.     GameManager game;
4.     private void Start()
5.     {
6.         game = FindObjectOfType(typeof(GameManager)) as GameManager;
7.     }
8.     private void OnTriggerEnter(Collider other)
9.     {
10.         if (other.tag == "Player")
11.         {
12.             game.Game();
13.             other.tag = "Dead";
14.             other.GetComponent<Collider>().isTrigger = true;
15.         }
16.  
17.     }
18. }

---------------------------------------------------------------------------------------------------------------------------------

Code (CSharp):

1. public class ObstacleCtrl : MonoBehaviour
2. {
3.     private void OnTriggerEnter(Collider other)
4.     {
5.         if (other.tag == "Player")
6.         {
7.             GameManager game = FindObjectOfType(typeof(GameManager)) as GameManager;
8.             game.ReTry();
9.         }
10.     }
11. }

 

A few things jump out at me.

GameManager: What is "temp"? Usually a name like "temp" is only used for a variable in a very limited scope, like within a single function. Here it's clearly not a temporary variable, as it appears to be set in Start, persist throughout the game, and never change.

General: There's a whole lot of GetComponent<> and friends here. Pretty much all of these should be called once and then stored on their relevant objects.

General: You use a lot of GameObject type variables. In general, if you know a type of component that will be on the object you want to reference, it's better to store the reference as that type. For example, on line 27 you know for sure that "temp" has a PlayerMove attached to it, but you store it as a GameObject; later on (say, line 45), you then have to call temp.GetComponent<PlayerMove> to get the component you really want. Part of this advice is efficiency (.gameObject and .transform are always faster than .GetComponent<>), but more importantly, it also makes it harder to store the wrong object in this variable.

GameManager lines 23-25: These things should be moved to a function on the PlayerMove class with a bool parameter. (Moving them there will not only prevent duplicate code like lines 75-76, but will also allow you to cache the component references on each PlayerMove object as described above)

GameManager line 33: This is minor but a nice convenience: string.Format is no longer necessary in most cases, you can use string interpolation now by putting a $ before the string:

Code (csharp):

1. tempTimer = $"{time:00}";

General: Tags suck, tbh. Because you can only have one tag, you can really only ever use tags for at most one type of thing, plus the fact that a tag exists doesn't guarantee any functionality on that object. It's generally better practice in those situations to check for the existence of a component rather than a tag

TargetTrigger and ObstacleControl: FindObjectOfType is quite slow. In these situations, a singleton pattern would be better:

Code (csharp):

1. public class GameManager : MonoBehaviour {
2. public static GameManager instance;
3.  
4. void Awake() {
5. instance=this;
6. }
7. }
8.  
9. ....
10.  
11. //this works anywhere else in your code
12. GameManager.instance.ReTry();

 

It's mostly fine, but maybe look at these:

• Dont make things public that dont have to be public. If you just want things to show up in the inspector use '[SerializeField] private' instead. This makes for a more robust architecture and is generally a better design practice.
• Find()... stop using it. You really never have to use Find or any of its variations. It's slow, as it has to go through the entire scene hierarchy to look for the desired object(s). It also doesnt scale well with project size for the same reason. Instead, pass things as references or input them through the inspector whenever possible. If something persists between scenes and you thus need to get a reference to it in the new scene, you could make the object a singleton.
• In PlayerMove you write transform.position += transform.forward / speed. So you are dividing by speed.. meaning more speed means slower movement? That doesnt make a whole lot of sense ^^
• I dont really know what you intend to do with the punch of left/right bools, but i dont exactly like the system.
• You dont need to compare bools to true of false. They are already a truth value. So 'right == true' is the same as 'right'. The negation, ie 'right == false' is the same as '!right', read as not right.
• Try to think about some more meaningful method names. I have no idea what 'Game()' is supposed to do. Not even with the comment. A method name, as a rule of thumb, should be some command. Something that happens. Like "SortList()". Everybody reading this name knows exactly what happens. Game() is pretty useless as far as information goes, and the others are only marginally better imho.

 

thanks a lot for you advices I'll follow these

 

thnaks a lot these will help my future coding

 

GameObject.Find I agree fully. FindObject(s)OfType occasionally has usefulness, if you need to iterate through a bunch of objects of a kind and can't modify that class's code to add a static List of those objects. It's rare, but not quite never.

(Also both are sometimes useful in editor code)

 

btw left/right bool is for buttonsi I attached that script to 2 buttons and just made each tru n the inspector

 

So I have several player game objects PlayerMove attached to them, we control them one by one after the last one reached to target. How can ı find them intead of using Find(); I mean can you sample it please?

 

Easiest thing to do is maintain a static List in PlayerMove:

Code (csharp):

1. public static List<PlayerMove> all;
2.  
3. void OnEnable() {
4. if (all == null) all = new List<PlayerMove>();
5. all.Add(this);
6. }
7. void OnDisable() {
8. all.Remove(this);
9. }

In Start, your GameManager can loop through this list like:

Code (csharp):

1. foreach (PlayerMove player in PlayerMove.all)

 

Ok I understood untill here and that made sense a lot and I think my last question is ı used int index to swtich between player objects in array so now how can I make this happen using list? Thanks a lot untill now and I hope I can get answer for this

 

I agree, even tho i didnt say it had no uses. I said it never has to be used, which should be true.
But yeah they have occassional uses i guess, then again it's a good rule of thumb for beginners to stay away from them to not develop hard to get rid of habbits imho. I personally use them for rapid prototyping, if anything. I cant say i developed a lot of editor scripts yet.

 

Hey what do you think of this now? looks better?

Code (CSharp):

1. public class GameManager : MonoBehaviour
2. {
3.     #region vars
4.     public static GameManager instance;
5.     PlayerMove[] players;
6.     public GameObject[] deadPoints;
7.     public GameObject[] enterancePoints;
8.     public GameObject enterance;
9.     public GameObject target;
10.     public GameObject playButton;
11.     public Text timerTxt;
12.     public float time;
13.     public float resetTime;
14.     string playerHoldTimer;
15.     public int nextSceneIndex;
16.     PlayerMove playerHold;
17.     int index=0;
18.     #endregion
19.     private void Awake()
20.     {
21.         instance = this;
22.     }
23.     void Start()
24.     {
25.         foreach(PlayerMove player in PlayerMove.all)
26.         {
27.             player.SetComponents(false);
28.             player.CanWeMove(false);
29.         }
30.         players = PlayerMove.all.ToArray();
31.         playerHold = players[index];
32.         StartMove();
33.     }
34.  
35.     void Update()
36.     {
37.         playerHoldTimer = $"{time:00}";
38.         time -= 1 * Time.deltaTime;
39.         timerTxt.text = playerHoldTimer;
40.         if (time <= 0)
41.         {
42.             ReTry();
43.         }
44.     }
45.     public void PlayGame() //Starts time and controls
46.     {
47.         playButton.SetActive(false);
48.         Time.timeScale = 1;
49.         playerHold.CanWeMove(true);
50.     }
51.     public void NextCar()//When reache to target changes car/enterance/target
52.     {
53.         int playerHoldLength = players.Length;
54.         playerHold.CanWeMove(false);
55.         index+=1;
56.         Debug.Log(index);
57.  
58.         if(index<playerHoldLength)
59.         playerHold = players[index];
60.  
61.         StartMove();
62.     }
63.     void StartMove()//prepares scene to next step
64.     {
65.         int playerHoldLength = players.Length;
66.         if (index == playerHoldLength)
67.         {
68.             SceneManager.LoadScene(nextSceneIndex);
69.             index = 0;
70.         }
71.  
72.         if (enterancePoints[index] != null)
73.             ChangeEnterance();
74.  
75.         SpawnPlayer();
76.         playerHold.SetComponents(true);
77.  
78.         if(deadPoints[index]!=null)
79.         target.transform.position = deadPoints[index].transform.position;
80.  
81.         ResetTimer();
82.         playButton.SetActive(true);
83.         Time.timeScale = 0;
84.      
85.     }
86.     public void ReTry()//when hits others
87.     {
88.         SpawnPlayer();
89.         playerHold.CanWeMove(false);
90.         ResetTimer();
91.         playButton.SetActive(true);
92.         Time.timeScale = 0;
93.     }
94.     void ResetTimer()
95.     {
96.         time = resetTime;
97.     }
98.     void SpawnPlayer()
99.     {
100.         playerHold.transform.position = enterance.transform.position;
101.         playerHold.transform.rotation = enterance.transform.rotation;
102.     }
103.     void ChangeEnterance()
104.     {
105.         enterance.transform.position = enterancePoints[index].transform.position;
106.         enterance.transform.rotation = enterancePoints[index].transform.rotation;
107.     }

---------------------------------------------------------------------------------------------------------------------

Code (CSharp):

1. public class PlayerMove : MonoBehaviour
2. {
3.     bool rightTurn;
4.     bool leftTurn;
5.     public float divideSpeed;
6.     public float rotationspeed;
7.     public bool canMove;
8.     public static PlayerMove instance;
9.     public static List<PlayerMove> all;
10.     private void Awake()
11.     {
12.         instance = this;
13.     }
14.     private void OnEnable()
15.     {
16.         if (all == null) all = new List<PlayerMove>();
17.         all.Add(this);
18.     }
19.     private void OnDisable()
20.     {
21.         all.Remove(this);
22.     }
23.     void Update()
24.     {
25.         if (canMove)
26.         {
27.             transform.position += transform.forward / divideSpeed;
28.  
29.             //// To test on PC
30.             if (Input.GetKey(KeyCode.RightArrow))
31.             {
32.                 transform.Rotate(Vector3.up * (rotationspeed * Time.deltaTime));
33.             }
34.             if (Input.GetKey(KeyCode.LeftArrow))
35.             {
36.                 transform.Rotate(Vector3.down * (rotationspeed * Time.deltaTime));
37.             }
38.             ////
39.             if (rightTurn)
40.             {
41.                 transform.Rotate(Vector3.up * (rotationspeed * Time.deltaTime));
42.             }
43.             if (leftTurn)
44.             {
45.                 transform.Rotate(Vector3.down * (rotationspeed * Time.deltaTime));
46.             }
47.         }
48.     }
49.     public void SetComponents(bool enable)  //to prepare car to game(to make it visible)
50.     {
51.         GetComponentInChildren<MeshRenderer>().enabled = enable;
52.         GetComponentInChildren<Collider>().enabled = enable;
53.     }
54.     public void CanWeMove(bool move)
55.     {
56.         canMove=move;
57.     }
58.     private void OnTriggerEnter(Collider other)
59.     {
60.         if (other.tag == "Player")
61.         {
62.             GameManager.instance.ReTry();
63.         }
64.     }
65.     public void TurnControl(bool left,bool right)
66.     {
67.         rightTurn = right;
68.         leftTurn = left;
69.     }
70. }

------------------------------------------------------------------------------------------------------------------

Code (CSharp):

1. public class TargetTrigger : MonoBehaviour
2. {
3.     private void OnTriggerEnter(Collider other)
4.     {
5.         if (other.tag == "Player")
6.         {
7.             GameManager.instance.NextCar();
8.             other.tag = "Dead";
9.             other.GetComponent<Collider>().isTrigger = true;
10.         }
11.  
12.     }
13. }

----------------------------------------------------------------------------------------------------------

Code (CSharp):

1. public class TurningApplier : MonoBehaviour, IPointerDownHandler, IPointerUpHandler
2. {
3.     public bool left;
4.     public bool right;
5.  
6.     public void OnPointerDown(PointerEventData eventData)//right and left turning control
7.     {
8.         if (right)
9.         {
10.          
11.            foreach (PlayerMove player in PlayerMove.all)
12.             {
13.                 player.TurnControl(false,true);
14.             }
15.         }
16.         if (left)
17.         {
18.             foreach (PlayerMove player in PlayerMove.all)
19.             {
20.                 player.TurnControl(true, false);
21.             }
22.         }
23.  
24.     }
25.     public void OnPointerUp(PointerEventData eventData)
26.     {
27.         foreach (PlayerMove player in PlayerMove.all)
28.         {
29.             player.TurnControl(false, false);
30.         }
31.     }
32. }
33.  

 

Better
You got rid of the comparing of bools to true or false, improved some of the names, and got rid of the Find()'s by using a singleton. That said, some things can still be improved, 'all', for example, is not a good name for a list of PlayerMove. Instead you could call it 'allMoves' or something like that.
You also tried to get rid of some of the GetComponents as suggested by StarManta, but you could still improve on that. In PlayerMove.SetComponents you are still overusing GetComponent. Instead, use GetComponent once in Start() and then enable or disable the saved references in your SetComponent method. Like so:

Code (CSharp):

1. public class PlayerMove : MonoBehaviour
2. {
3.     private MeshRenderer meshRenderer;
4.     private Collider collider;
5.  
6.     void Start(){
7.         meshRenderer = GetComponentInChildren<MeshRenderer>();
8.         collider = GetComponentInChildren<Collider>();
9.     }
10.  
11.     public void SetComponents(bool enable)  //to prepare car to game(to make it visible)
12.     {
13.         meshRenderer.enabled = enable;
14.         collider.enabled = enable;
15.     }
16. }

Now you only need to look for the component once and can then quickly enable or disable it through the previously found reference. Generally speaking, saving references you would otherwise have to get more than once is a good idea.

By the way, in OnTriggerEnter you reiceve (Collider other) as parameter. So you dont need to write other.GetComponent<Collider>, since other already is a collider. Little things like these are just unnecessarily wasting performance. I'm pretty sure there are other little mistakes like that, or that the solution for whatever game you are writing could probably be implemented in a shorter way.
That said, for the most part the code looks ok now, and you will learn to avoid little mistakes as you get more experience.

 

Thanks alot for your time and advices. I learned actually got the points of stomehings better now.