Classes and scripting organization

Hello everybody,
I'm a new Unity scripter. After reading the beginner and intermediate lessons about scripting, after tutorials, I want to create a simple game just to have a little practice.

This game will be a tapping one. You have to click (or to tap on mobile) the "screen" to kill one by one monsters.

I have no graphics (my enemies are just a cube, a sphere, etc...) because I just want to script.

I've started with simple things and a little bit I'm adding stuff.

Actually I need an advice because I think that my scripts (two) are too disorganized.

I have two scripts one is for prefab enemis (called enemy.cs) and the other one is for managing these enemies and other things like coins ( that will be needed to improve the player damage), but because I read that a class should only do one thing and only one, well I need this advice from you.

Code (CSharp):

1. using UnityEngine;
2. using System.Collections;
3.  
4. public class Enemy : MonoBehaviour {
5.  
6.     public int enemyHealth = 10;                    // Base enemy life
7.  
8.     static int count = 0;                            // Counter to know how many monster have been created (and which is the difficult level
9.     static float gK = 0.015f;                        // Const used to raise monster life
10.     static int coin = 10;                            // How many coins the monster will drop, actually it does not increase.
11.     static ManagingEnemies managingEnemies;            // Here will be the reference to the ManagingEnemies script.
12.  
13.     void Awake ()
14.     {
15.         managingEnemies = GameObject.FindGameObjectWithTag("GameController").GetComponent<ManagingEnemies>();            // Take reference to the other scripts
16.     }
17.     void Start ()
18.     {                                  
19.         enemyHealth = EnemyLife ();                                                    // On start the it will be assigned the enemy life as the function below does.
20.  
21.     }
22.  
23.     private int EnemyLife()
24.     {
25.         float tempH = enemyHealth * ( 1 + NumberPower((count++ - 1),2) * gK );        // The function which calculate the increasing of enemy's life
26.         return (int)tempH;
27.     }
28.  
29.  
30.     public void TakeDamage(int damage)                                                //Function which manage the damage took.
31.     {
32.         if (enemyHealth > 0)
33.             enemyHealth -= damage;
34.         else if(enemyHealth <= 0)
35.             Death();
36.      }
37.  
38.     private float NumberPower(float number, int index)                                // exponentiation function. I dunno if there is already one.
39.     {
40.         for (int i = 0; i < index; i++)
41.             number *= number;
42.         return number;
43.     }
44.  
45.     private void Death()                                                            //When the monster dies, this function gives the coin to the player and destroy the old monster.
46.     {
47.         Destroy (gameObject);
48.         managingEnemies.AddCoin (coin);
49.  
50.     }
51.  
52.  
53.  
54.  
55. }
56.  

Code (CSharp):

1. using UnityEngine;
2. using System.Collections;
3. using UnityEngine.UI;
4.  
5. public class ManagingEnemies : MonoBehaviour {
6.  
7.     public GameObject[] enemyObj;                 // Here there is an array with monster prefab. On each monster prefab there is an enemy.cs script
8.     public Text text;                            // Text item to count the coins
9.  
10.  
11.     GameObject enemyInstance;
12.  
13.     private int damage = 1;                        // Actual damage
14.     private int coin = 0;                        // Starting coin
15.     private Enemy enemy;                        // Variable to get the reference to the enemy script
16.  
17.     void Awake ()                                 // On Awake I take the reference to the text item and I update it
18.     {
19.         text = GameObject.Find ("Coin").GetComponent<Text> ();
20.         UpdateCoinText ();
21.  
22.     }
23.        
24.     private void UpdateCoinText()                 // Function created to update coin text
25.     {
26.         text.text = "Coin: " + coin;
27.     }
28.  
29.     void Update()                                // Each update if there is no enemy instance a new one will be created, if player tapped call the Tap() function
30.     {
31.         if (enemyInstance == null)
32.             CreateEnemy ();          
33.         if (CheckTap ())
34.             Tap ();
35.        
36.     }
37.  
38.     void CreateEnemy()                            // As the name says, this function creates a new enemy instance and takes the reference to its script.
39.     {
40.         enemyInstance = Instantiate (enemyObj[Random.Range (0, enemyObj.Length)], new Vector2 (0, 0), Quaternion.identity) as GameObject;             // Instantiate a random monster among those referenced.
41.         enemy = enemyInstance.GetComponent <Enemy> ();
42.  
43.     }
44.    
45.     private bool CheckTap()                        // Simple...but..maybe useless?
46.     {
47.         if (Input.GetMouseButtonDown (0))
48.             return true;
49.         return false;
50.     }
51.  
52.     private void Tap()                            // If the enemy reference is not null, the enemy takes damage, using enemy class
53.     {
54.  
55.         if (enemy != null)
56.             enemy.TakeDamage(damage);  
57.  
58.     }
59.  
60.     public void AddCoin(int amount)                // Functions to add or subtract coins.
61.     {
62.         coin += amount;
63.         UpdateCoinText ();
64.     }
65.  
66.     public bool SubCoin(int amount)
67.     {
68.         if (amount >= coin) {
69.             coin -= amount;
70.             UpdateCoinText ();
71.             return true;
72.         }
73.         return false;
74.     }
75.  
76.  
77.  
78.  
79.    
80.  
81.  
82. }
83.  

Maybe this is a long post....but please, I really want to improve my skills...
Thank you

 

If you think your code is too disorganized, and you are having trouble reading it, then it probably is. I use a combination of things to help organize my code a little better. Basically, what you want to do is group similar things into regions and note their usage with comments. Moreover, separate things that are related even further into different classes.

 

You dont appear to be using enemyObj?

 

Not sure that has anything to do with the OP's question of how to organize code

 

I often have scripts hundreds of lines long, so I use certain rules regarding indentation and placement of brackets/curly braces and ellipses to help my code look better and be more navigable and easy to read. Too little space can make it very difficult to find what you are looking for, and too much is also hard to grasp.

#1: For brackets/curly braces, if there are more than four lines of code within the brackets, I place the opening bracket on the next line, else, I put it at the end of the same line with no space.
#2: I always put one empty line in between separate thoughts, while keeping related statements grouped together.
#3: I basically just try to follow the same practices for everything. It becomes another type of syntax, but an aesthetic syntax.
#4: Last but not least, just code a lot. It will come to you in time, and you will develop your own set of aesthetic grammar rules that make your workflow fast and efficient.

Here is a short example of how I structure my code.

Code (CSharp):

1. // over four lines
2. foreach(Type individual in group)
3. {
4.    // under five lines
5.    if(individual == testItem){
6.       string text = "Found Item: " + individual.ToString();
7.       Debug.Log(text);
8.    }
9.  
10.    // single line, no brackets required
11.    if(individual == null) return;
12.  
13.    // double line, no brackets required, but more space due to the amount of information being handled
14.    if(individual != null  && individual != testItem && testItem != null)
15.       Debug.Log("Individual is not equal to " + testItem + ".");
16. }

 

At the risk of starting a flame war - If statements without braces are in my opinion never a good thing. The only exception being when they are followed by a return.

In your third example, what happens if you wanted to add an extra debug line or something?

 

also you should get rid of the visual clutter of commenting too much, no point in commenting about what is obvious about things via the Method and Variable names. Comments should be more about reminding you of your intention behind things, not restating what things are.

 

I do like the other guys code but I agree with the braces. I always use braces even if they only enclose one line. It's a personal preference thing though. I do it for the reason you mentioned. What if I want to add to that block?

 

Then you simply add braces.

 

Thanks a lot for all your comments.

Well I don't think that the code is disorganized but I have fear of it

I used to be a programmar in other languages long time ago (about 6/7 years ago) and I never knew if my way of thinking in OOP is ok or not...I just wanted to have a comparison with other programmers that are better then me

 

What an astute observation. That's why I add them from the start to avoid the hassle of having to navigate around the line that is already there. Adding them from the start saves me at least a second in time for each block I need to go back and add braces to later. I'm all about efficiency haha

 

but there are also a lot of situations when you know you will not need more than 1 line in the conditional. Such as for a return, or if null checks. Also a lot of cases when a ternary can be used in place of a if.

 

The thing is, braces are explicit - the code in here will only be executed if this statement is true.
Clear code is much better than saving what, one line? (If you do Java style braces) When browsing over a lot of code (believe me I have done this) and are trying to get a grasp of it, it is a lot easier to see program flow. Braces aid understanding of flow. If you develop in a larg(ish) team you will really understand this.

The only exceptions are conditionals that return immediately (old school null pointer checks in c++ come to mind) and conditionals that break loops.

 

It's really just my personal preference. I format my code by starting the braces on the same line as my statement so it doesn't really add any clutter and it only adds one extra line. It's easier for me to read that way. Nobody formats their code exactly the same as someone else.

 

#nailedit

 

more like indentation, its far faster and easier to understand the spatial information than it is to watch for braces. When it comes to trying to wrap the human mind around the flow.

At least we can agree on there is no point in commenting the obvious that can be understood by using good names.

 

Yeah, probably a combination of both I thing. But yeah this is a hot topic, with many opinions right! Comments I agree on though. Comment should by why, not what.

 

Alright, it's clear that this is just going to start a code formatting war. I'm finished. It's like a religion; everyone prays to their own formatting god because you were raised on His teachings or find them easier to follow. I think it's best if we just say we're all right and not spill any blood over this. Point is, do what works for you. Organize it to the best of your ability and any other programmer is going to be able to read it; programmers aren't stupid.

 

its because it comes from peoples roots if you got a java, c++ or say a ruby or python background. Really influences how people like to do things.

The same war could be started between implicit or explicit in many other things like the encapsulation level.
Whats important is your intent can be understood, and that your team works well together.

Keep the intent clear, comment the intent if needed. Keep good encapsulation, and make sure you got 1 responsibility per class.

 

Agreed. I started out programming for web many years, so I tend to format things in more of a Javascript style, though I never use Javascript anymore because I prefer C#.

 

You do have a point there, and yes, that second example of no brackets required is probably the most complex that I would allow a conditional to get before I put braces in. I think of one line of code as one sentence. If I can express a conditional as one sentence without it becoming too long or confusing, I will. If I can tell what it is doing at a glance, I will leave it as simple as possible.
But you're right. In cases where I am going to end up having to add more code to the conditional, I will put in braces.

 

Because we are already here I ask a thing...

Code (CSharp):

1.     void Update()                                // Each update if there is no enemy instance a new one will be created, if player tapped call the Tap() function
2.     {
3.         if (enemyInstance == null) {
4.             CreateEnemy();
5.         }
6.         if (CheckTap ())
7.             Tap ();
8.        
9.     }

About this code...I wish I could wait for some seconds before creating a new enemy so I changed the code in this way

Code (CSharp):

1.     void Update()                                // Each update if there is no enemy instance a new one will be created, if player tapped call the Tap() function
2.     {
3.         if (enemyInstance == null) {
4.             Invoke("CreateEnemy",2);
5.         }
6.         if (CheckTap ())
7.             Tap ();
8.        
9.     }

But now at the start he always calls the "CreateEnemy" function.....he creates a lot of instance....

I tried even to change the return type for CreateEnemy from void to IEnumerator to use the yield with "WaitForSeconds" but he always returns back....suggestions?

 

Does the CreateEnemy() method populate the enemyInstance variable? You are checking if that variable is null and then creating a new enemy if it is. If it is creating new enemies on update perhaps you are not setting the variable.

 

I think I do....
Anyway the actual code the "CreateEnemy()" is this.

Code (CSharp):

1.     void CreateEnemy()                            // As the name says, this function creates a new enemy instance and takes the reference to its script.
2.     {
3.         enemyInstance = Instantiate (enemyObj[Random.Range (0, enemyObj.Length)], new Vector2 (0, 0), Quaternion.identity) as GameObject;             // Instantiate a random monster among those referenced.
4.         enemy = enemyInstance.GetComponent <Enemy> ();
5.     }
6.    

 

That is happenning because you are delaying two seconds before creating the first enemy. In those two seconds before CreateEnemy is called, Update is probably called about 120 times (think 60 fps). In those 120 times enemyInstance is still null!

 

Looking back to code in the OP, I see a lot of dangerous practices. Take it with a grain of salt, I'm only looking at an isolated section of your code base.

• Static variables. These will turn around and bite you.
• Static variables you are reassigning in Awake. If you must use a static variable, only assign it once.
• ++ used in the middle of an equation. While technically legal, its just asking for confusion
• Extraneous comments. Comments should be used sparingly in cases where the intent of the code is not clear. A variable name should stand for itself. totalCountOfEnemies is better then count // total count of enemies
• Casting instead of rounding. Again, not technically wrong, but I would only suggest it as an optimization in super hot code
• Rewriting of Mathf functions
• You've got a lot of tiny methods. Its not actually obvious from reading the names of the method what the method does. If this is as complex as the code is going to get, I'd actually suggest in lining them for readability. On the other hand if the tap method is going to get much more complex I'd suggest renaming it to make it obvious what it does.

Most of this is just stuff that I found jarring when reading your code as a stranger. You should assume a total stranger will be reading your code, even if it is only you in six months. Some of it might just be style differences, but some of it might be real.

 

-.-''' I didn't think to this...so I have to find a way to overcome this problem...

THANKS A LOT!!!

I changed a bit my code following your suggestions...to go point by point...

• About static variables. I declared those because I want them attached only to the class and not the istances...I dunno how they will "bite me"
• I changed this variable removing the static, to avoid to reassignig each time
• Ok =)
• I made these comments to explain my code here and to avoid that someone could tell me "Comment the code if you want we read it" - I renamed some variables and functions name to make them more obvious
• Read below
• I discovered Mathf functions =P Thank you Now I don't cast anymore but I rounded the value from float to int (Mathf.RoundToInt) and I used Mathf.Pow instead of rewriting an existing function
• As wrote above I changed names

Thank you very very very very much! I learned a lot from you =)

 

Just wait. I could be wrong. But I've generally regretted statics in the past. That said, if they meet you current needs then use them.

I was thinking more along the lines of this. Since you are using a static variable then you only need to do the Find call on the first time Awake is run. If you already know about the manager on a second enemy there is no need to look for it.

Code (CSharp):

1. static ManagingEnemies managingEnemies;        
2.  
3.     void Awake ()
4.     {
5.         if(!managingEnemies){
6.             managingEnemies = GameObject.FindGameObjectWithTag("GameController").GetComponent<ManagingEnemies>();  
7.         }
8.     }
9.  

Getting the balance right on comments can be tricky. It gets easier with practice to write self documenting code.

 

If you only have one instance of ManaginEnemies, you could move the finding of the instance from Awake to a getter:

Code (CSharp):

1. private static ManagingEnemies managingEnemiesBackingField;
2. private static ManagingEnemies managingEnemies {
3.     get {
4.         if(managingEnemiesBackingField == null)
5.             managingEnemiesBackingField = GameObject.Find....
6.         return managinEnemiesBackingField;
7. }

That'll mean that you won't start looking for the object before you need it. It's not really better or prettier in any objective way - I just like that pattern for references to other objects.


Another alternative is to turn the ManagingEnemies class into a singleton:

Code (CSharp):

1. public class ManagingEnemies : MonoBehaviour {
2.  
3.     public static ManagingEnemies instance {get; private set; }
4.  
5.     void Awake() {
6.           if(instance != null)
7.               Destroy(this);
8.           else
9.                instance = this;
10.      }
11.      ....
12. }
13.  
14. //remove managingEnemies field in Enemy.cs, and reference the static instance variable instead:
15.  
16.     private void Death() {
17.       Destroy(gameObject);
18.       ManagingEnemies.instance.AddCoin(coin);
19.    }
20.