Beginner Scripting, please help and check if its alright?

Hey guys! I recently started to script using c# and i made 5 scripts! However, I really want someone to check over the scripts to see if I can improve anything.

Code (CSharp):

1. using UnityEngine;
2. using System.Collections;
3.  
4. public class EnemyAI : MonoBehaviour {
5.     public Transform target;
6.     public int movespeed;
7.     public int rotationspeed;
8.     public int maxDistance;
9.     private Transform myTransform;
10.  
11.     void Awake() {
12.         myTransform = transform;
13.     }
14.  
15.     // Use this for initialization
16.     void Start () {
17.         GameObject go = GameObject.FindGameObjectWithTag ("Player");
18.  
19.         target = go.transform;
20.  
21.         maxDistance = 2;
22.  
23.     }
24.    
25.     // Update is called once per frame
26.     void Update () {
27.         Debug.DrawLine (target.position, myTransform.position,Color.yellow);
28.  
29.         //Look at target
30.         myTransform.rotation = Quaternion.Slerp (myTransform.rotation, Quaternion.LookRotation(target.position - myTransform.position), rotationspeed * Time.deltaTime);  
31.  
32.         if(Vector3.Distance(target.position, myTransform.position) > maxDistance){
33.         //Move towards target
34.         myTransform.position += myTransform.forward * movespeed * Time.deltaTime;
35.     }
36. }
37. }

Code (CSharp):

1. using UnityEngine;
2. using System.Collections;
3.  
4. public class EnemyAttack : MonoBehaviour {
5.     public GameObject target;
6.     public float attackTimer;
7.     public float coolDown;
8.    
9.     // Use this for initialization
10.     void Start () {
11.         attackTimer = 0;
12.         coolDown = 2.0f;
13.     }
14.    
15.     // Update is called once per frame
16.     void Update () {
17.         if (attackTimer > 0)
18.             attackTimer -= Time.deltaTime;
19.        
20.         if (attackTimer < 0)
21.             attackTimer = 0;
22.  
23.             if (attackTimer == 0) {
24.                 Attack ();
25.                 attackTimer = coolDown;
26.             }
27.         }
28.  
29.     private void Attack(){
30.         float distance = Vector3.Distance (target.transform.position, transform.position);
31.        
32.         Vector3 dir = (target.transform.position - transform.position).normalized;
33.        
34.         float direction = Vector3.Dot (dir, transform.forward);
35.        
36.         Debug.Log (distance);
37.        
38.         if (distance < 2.5) {
39.             if(direction > 0) {
40.                 PlayerHealth eh = (PlayerHealth)target.GetComponent ("PlayerHealth");
41.                 eh.AddjustCurrentHealth (-10);
42.             }
43.         }
44.     }
45.    
46. }

Code (CSharp):

1. using UnityEngine;
2. using System.Collections;
3.  
4. public class PlayerAttack : MonoBehaviour {
5.     public GameObject target;
6.     public float attackTimer;
7.     public float coolDown;
8.  
9.     // Use this for initialization
10.     void Start () {
11.                 attackTimer = 0;
12.                 coolDown = 2.0f;
13.     }
14.  
15.     // Update is called once per frame
16.     void Update () {
17.         if (attackTimer > 0)
18.                         attackTimer -= Time.deltaTime;
19.  
20.         if (attackTimer < 0)
21.                         attackTimer = 0;
22.  
23.         if (Input.GetKeyUp(KeyCode.F)) {
24.                         if (attackTimer == 0) {
25.                                 Attack ();
26.                                 attackTimer = coolDown;
27.             }
28.         }
29. }
30.         private void Attack(){
31.         float distance = Vector3.Distance (target.transform.position, transform.position);
32.  
33.         Vector3 dir = (target.transform.position - transform.position).normalized;
34.  
35.         float direction = Vector3.Dot (dir, transform.forward);
36.  
37.         Debug.Log (distance);
38.  
39.         if (distance < 2.5) {
40.             if(direction > 0) {
41.             EnemyHealth eh = (EnemyHealth)target.GetComponent ("EnemyHealth");
42.             eh.AddjustCurrentHealth (-10);
43.                 }
44.     }
45. }
46.  
47. }

Code (CSharp):

1. using UnityEngine;
2. using System.Collections;
3.  
4. public class EnemyHealth : MonoBehaviour {
5.     public int maxHealth = 100;
6.     public int curHealth = 100;
7.    
8.     public float healthBarLengh;
9.    
10.     // Use this for initialization
11.     void Start () {
12.         healthBarLengh = Screen.width / 2;
13.     }
14.    
15.     // Update is called once per frame
16.     void Update () {
17.         AddjustCurrentHealth(0);
18.     }
19.    
20.     void OnGUI() {
21.         GUI.Box(new Rect(10, 40, healthBarLengh, 20), curHealth + "/" + maxHealth);
22.     }
23.    
24.     public void AddjustCurrentHealth(int adj) {
25.         curHealth += adj;
26.        
27.         if(curHealth < 0)
28.             curHealth =  0;
29.        
30.         if(curHealth > maxHealth)
31.             curHealth = maxHealth;
32.        
33.         if(maxHealth < 1)
34.             maxHealth = 1;
35.        
36.         healthBarLengh = (Screen.width / 2) * (curHealth / (float)maxHealth);
37.     }
38. }

Code (CSharp):

1. using UnityEngine;
2. using System.Collections;
3.  
4. public class PlayerHealth : MonoBehaviour {
5.     public int maxHealth = 100;
6.     public int curHealth = 100;
7.    
8.     public float healthBarLengh;
9.    
10.     // Use this for initialization
11.     void Start () {
12.         healthBarLengh = Screen.width / 2;
13.     }
14.    
15.     // Update is called once per frame
16.     void Update () {
17.         AddjustCurrentHealth(0);
18.     }
19.    
20.     void OnGUI() {
21.         GUI.Box(new Rect(10, 10, healthBarLengh, 20), curHealth + "/" + maxHealth);
22.     }
23.    
24.     public void AddjustCurrentHealth(int adj) {
25.         curHealth += adj;
26.        
27.         if(curHealth < 0)
28.             curHealth =  0;
29.        
30.         if(curHealth > maxHealth)
31.             curHealth = maxHealth;
32.        
33.         if(maxHealth < 1)
34.             maxHealth = 1;
35.        
36.         healthBarLengh = (Screen.width / 2) * (curHealth / (float)maxHealth);
37.     }
38. }

If anyone is generous enough to look over some short scripts, and tell me where I could improve it, I would be very grateful. Thanks very much !!!

 

Couple of things if I may. (And please remember that all of this is "in the eye of the beholder").

1. You have a number of monobehaviours that characterize various aspects of enemy behavior. I would create an Enemy monobehaviour and keep all of this code there. The same applies to player. So at the end I would have just 2 classes - Enemy and Player. Your Update method would look something like:

Code (CSharp):

1. void Update ()
2. {
3.         move();
4.         attack();
5. }
6.  

where move() would do what your EnemyAI currently does (which I would BTW generalize into some kind of AI class as a "pursuit" AI behavior) and attack() would do what your EnemyAttack currently does.

2. In PlayerAttack you are directly accessing a Health component.If you follow #1 then you should create a public method like hit() on Enemy class. That will give you a far greater control of how the damage is applied. For example, if your enemy has an additional defense. That makes it transparent to the player since Player class should not know any internals of the Enemy class.

3. As a bit of a nitpicking. In your EnemyAI class you try to turn the enemy towards the target unconditionally. Try to come up with a condition. For example, the difference between normalized look and direction to target vectors should be more than some min value.

4. As a personal preference I usually have a Game class that knows about and to certain degree governs all objects (player, enemies, etc...) in the game. In your situation I would register my player and enemies with this kind of a class so your objects don't need to do any kind of searches by tag or getComponent calls inside the Update. It is not efficient.

You are of course free to disagree with any of this. Like I said - "in the eye of the beholder".

 

Thanks you so much!
After reading your reply, i know a lot of areas i could improve!

 

Just to provide a different perspective, I like the way you've divided different aspects of behavior into different scripts. I used to have "player" and "enemy" scripts, as @cdevl describes, but in recent times I've grown to really embrace component-oriented architecture (which leads to different components handling different aspects of behavior).

For example, as it is, you can have different kinds of attacks, and different kinds of pursuits, and mix and match. You could have a stationary tower that can't move, but still attacks the player if the player gets close enough. Or you could have something that chases the player but doesn't attack. I like the flexibility you get from separating these.

On the other hand, you might consider combining EnemyHealth and PlayerHealth into a single Health script that works for anybody. They should be pretty much the same, at least until the health goes to zero and you need to handle death of that unit. But then you could have a separate Death component that you find and invoke to handle the death of a unit in whatever way is appropriate — instantiating a particle effect, removing the unit, or declaring Game Over.

As cdevl said, these are all mere suggestions, which you should feel free to ignore. You are off to a great start; already you are writing better Unity scripts than many people who have been at it longer.

There is one trick you should know about: you can subclass monobehaviours. For example, if needed, you could make a PlayerHealth class that derives from your Health class (instead of deriving from MonoBehaviour directly), and does whatever special/different stuff is needed to handle the player's health. In other scripts, when you do GetComponent<Health>, it will return any component that is of type Health or any subclass of Health, such as PlayerHealth.

If any of this is unclear, just say so and I'll explain in more detail. 頑張ってください！

 

Totally agree with Joe Strout about subclassing monobahaviours. That is an absolutely essential technique to adopt. Based on my suggestion you can have a base Character Monobehaviour, for example, that may contain functionality like basic attack and basic reaction to being hit (assuming that a player and an enemy have a similar nature).

just to save you a bit of a headache. When subclassing Monobehaviours declare all your Unity standard methods with a public qualifier or you may have issues.

 

My rules -

1. Functions that describe very different behaviors done by the same Object should still be in different scripts (components).
2. Functions that describe very similar behaviors done by different objects should, at the very least, inherit from the same script.

In your case, you have 3 scenarios/relationships to look at - Enemy Attack and EnemyAI (move), EnemyAttack and PlayerAttack, and EnemyHealth and Player Health.

I like that EnemyAttack and EnemyAI are separate scripts. They do not talk to each other and manage completely different behaviors. It allows you to, as JoeStrout said, mix and match your movement and attack methods.

PlayerHealth and EnemyHealth differ in only two cases - who they are describing, and where on the screen they are located. I would make a generic Health script, and assign one to the enemy and one to the player. Make your screen height/position a parameter. Now, your attack functions need only to say GetComponent<Health> and do their magic with that script.

The attack methods have a number of approaches - I think what you are doing works, but there are a number of ways of improving it depending on the direction you want to take. I feel like I could ramble on and on about it, so I'll stop here. PM if you want more.

Overall a strong start IMO. I say keep at it!

 

THANKS GUYS !
I really enjoyed reading all the different replies!
THANKS SO MUCH FOR HELPING ME OUT !