Seeking script feedback - best practices

Hi Guys,
Recently I've been trying to focus on my scripting and trying to improve the way that I implement ideas into a script. I've gotten some flack recently on some of my scripts and am seeking feedback on anything that could be improved, simplified, or any programming 'best-practices' in general that I should implement (or avoid). I would appreciate any feedback anyone has on my most recent script, and am open to any ideas or improvements that can help me improve. Does anything here jump out at you as bad? The script is a modified player movement script to allow for boosting and dashing.

Thanks in advance

Code (CSharp):

1. using UnityEngine;
2. using System.Collections;
3.  
4. public class BoostMovement : MonoBehaviour {
5.     public float speed = 6;
6.     public float jumpSpeed = 8;
7.     public float gravity = 25;
8.     public float boostForce = 18;
9.     public float groundDashForce = 18;
10.     public float airDashForce = 33;
11.  
12.     float mass = 1;
13.     float dashTimer = 0;
14.     float dashDelay = 0.7f;
15.  
16.     public bool isGrounded = true;
17.     public bool usedBoost = false;
18.  
19.     private Vector3 impact = Vector3.zero;
20.     private Vector3 moveDirection = Vector3.zero;
21.  
22.     CharacterController controller;
23.  
24.     void Start () {
25.         controller = GetComponent <CharacterController> ();
26.     }
27.    
28.     void Update ()
29.     {
30.         //update collision flags to determine if the controller is touching the ground
31.         CollisionFlags flags = controller.Move (moveDirection * Time.deltaTime);
32.         isGrounded = (flags & CollisionFlags.CollidedBelow) != 0;
33.  
34.         if(isGrounded)
35.         {
36.             //can only move control direction while on ground
37.             moveDirection = new Vector3(Input.GetAxis("Horizontal"), 0, Input.GetAxis("Vertical"));
38.             moveDirection = transform.TransformDirection(moveDirection);
39.             moveDirection *= speed;
40.  
41.             if (Input.GetKeyDown ("space"))
42.             {
43.                 moveDirection.y = jumpSpeed;
44.             }
45.             //boost can be used once per jump, resets on ground
46.             usedBoost = false;
47.         }
48.         else if(!isGrounded)
49.         {
50.             if(Input.GetKeyDown ("space") && !usedBoost)
51.             {
52.                 Boost ();
53.             }
54.         }
55.  
56.         //wait for player to press left shift to dash, can be done in air or on ground
57.         if(Input.GetKeyDown("left shift") && dashTimer <= 0)
58.         {
59.             Dash ();
60.         }
61.         //apply impact force on the controller
62.         if (impact.magnitude > 0.2F)
63.         {
64.             controller.Move(impact * Time.deltaTime);
65.         }
66.  
67.         // consumes the impact energy each cycle
68.         impact = Vector3.Lerp(impact, Vector3.zero, 5 * Time.deltaTime);
69.  
70.         // Apply gravity
71.         moveDirection.y -= gravity * Time.deltaTime;
72.  
73.         //cool off timer for dash
74.         if(dashTimer > 0)
75.             dashTimer -= Time.deltaTime;
76.  
77.     }
78.     void Dash()
79.     {
80.         //dash can always be used in the air, but only sideways and backwards when on the ground
81.         if(!isGrounded || (isGrounded && (Input.GetKey(KeyCode.A) || Input.GetKey(KeyCode.S) || Input.GetKey(KeyCode.D))))
82.         {
83.             moveDirection = new Vector3(Input.GetAxis("Horizontal"), 0, Input.GetAxis("Vertical"));
84.             moveDirection = transform.TransformDirection(moveDirection);
85.             AddImpact(new Vector3 (moveDirection.x,0,moveDirection.z), isGrounded ? groundDashForce : airDashForce);
86.             dashTimer = dashDelay;
87.         }
88.     }
89.  
90.     void Boost()
91.     {
92.         usedBoost = true;
93.         moveDirection.y = boostForce;
94.     }
95.  
96.     void Awake ()
97.     {
98.         CharacterController controller = GetComponent<CharacterController>();
99.         if(!controller)
100.             gameObject.AddComponent ("CharacterController");
101.     }
102.  
103.     public void AddImpact(Vector3 dir, float force)
104.     {
105.         dir.Normalize();
106.         if (dir.y < 0)
107.             dir.y = -dir.y; // reflect down force on the ground
108.         impact += dir.normalized * force / mass;
109.     }
110.  
111. }

 

Honestly, not bad at all. I've gone through and done a quick refactor, and made comments from your original to the refactored version. I removed your comments to avoid confusion. If anything is unclear, let me know.

Annotated code:

Code (CSharp):

1. using UnityEngine;
2. using System.Collections;
3.  
4. public class BoostMovement : MonoBehaviour {
5.     /* All of these are public, which is absolutely fine if you want
6.      * them to be accessible from the Unity editor.
7.      *
8.      * Quite frequently, I like to group like-properties into their own struct or class,
9.      * Especially in large projects. See Fig 2.2
10.      */
11.     public float speed = 6;
12.     public float jumpSpeed = 8;
13.     public float gravity = 25; // <- See Fig 2.1
14.     public float boostForce = 18;
15.     public float groundDashForce = 18;
16.     public float airDashForce = 33;
17.  
18.     float mass = 1;
19.  
20.     /* You might want to make your own time tracking class, so you can use it in other objects as well.
21.      * See Fig 1
22.      */
23.     float dashTimer = 0;
24.     float dashDelay = 0.7f;
25.  
26.     /* I feel like these should both be properties, rather than fields. They're assignable
27.      * from outside code and that is fundamentally insecure.
28.      * See Fig 2.3
29.      */
30.     public bool isGrounded = true;
31.     public bool usedBoost = false;
32.  
33.     private Vector3 impact = Vector3.zero;
34.     private Vector3 moveDirection = Vector3.zero;
35.  
36.     CharacterController controller;
37.  
38.     //I would use Awake() for this initialization rather than Start().
39.     //Here's why: http://unity3d.com/learn/tutorials/modules/beginner/scripting/awake-and-start
40.     void Start () {
41.         controller = GetComponent <CharacterController> ();
42.     }
43.  
44.     void Update ()
45.     {
46.         /*
47.          * If a function does many things, i like to split it up into smaller functions so that it's easier to follow.
48.          * Especially in the update loop, because it's such an important loop.
49.          * See Fig 3.1
50.          */
51.         CollisionFlags flags = controller.Move (moveDirection * Time.deltaTime);
52.         isGrounded = (flags & CollisionFlags.CollidedBelow) != 0;
53.    
54.         if(isGrounded)
55.         {
56.             moveDirection = new Vector3(Input.GetAxis("Horizontal"), 0, Input.GetAxis("Vertical"));
57.             moveDirection = transform.TransformDirection(moveDirection);
58.             moveDirection *= speed;
59.  
60.             /* GetKeyDown is fine, but if you use Input.GetButton instead, you can define
61.              * custom user input in Edit -> Project Settings -> Input.
62.              *
63.              * That way, you can change the button you want to be used for jump or even let players
64.              * define their own buttons. See Fig 1
65.              */
66.             if (Input.GetKeyDown ("space"))
67.             {
68.                 moveDirection.y = jumpSpeed;
69.             }
70.  
71.             usedBoost = false;
72.         }
73.  
74.         else if(!isGrounded)
75.         {    /* These two conditions can be combined.
76.              * See Fig 3.2 */
77.             if(Input.GetKeyDown ("space") && !usedBoost)
78.             {
79.                 Boost ();
80.             }
81.         }
82.  
83.         if(Input.GetKeyDown("left shift") && dashTimer <= 0)
84.         {
85.             Dash ();
86.         }
87.  
88.         if (impact.magnitude > 0.2F)
89.         {
90.             controller.Move(impact * Time.deltaTime);
91.         }
92.    
93.         impact = Vector3.Lerp(impact, Vector3.zero, 5 * Time.deltaTime);
94.  
95.         //Should gravity only be applied if isGrounded? Fig 4
96.         moveDirection.y -= gravity * Time.deltaTime;
97.    
98.         //Notice that, using the timer class in the refactored code, you don't need to decrement the timer yourself.
99.         if(dashTimer > 0)
100.             dashTimer -= Time.deltaTime;
101.    
102.     }
103.  
104.     //In the refactor, I combined dash into the ApplyUserInput function. See Fig 3.5
105.     void Dash()
106.     {
107.         // This condition might be a bit more complicated than it needs to be. I'm assuming you're using A S and D
108.         // as left movement, right movement, and back movement. As it turns out, these keys also correspond with
109.         // the axis values. A much simpler condition would be (isGrounded and vertical <= 0f) See Fig 3.4
110.         if(!isGrounded || (isGrounded && (Input.GetKey(KeyCode.A) || Input.GetKey(KeyCode.S) || Input.GetKey(KeyCode.D))))
111.         {
112.             //There's a bit of redundancy here, i've cleaned it up, See Fig 3.6
113.             moveDirection = new Vector3(Input.GetAxis("Horizontal"), 0, Input.GetAxis("Vertical"));
114.             moveDirection = transform.TransformDirection(moveDirection);
115.             AddImpact(new Vector3 (moveDirection.x,0,moveDirection.z), isGrounded ? groundDashForce : airDashForce);
116.  
117.             dashTimer = dashDelay;
118.         }
119.     }
120.  
121.     //In the refactor, I combined boost into the ApplyUserInput function. See Fig 3.3
122.     void Boost()
123.     {
124.         usedBoost = true;
125.         moveDirection.y = boostForce;
126.     }
127.  
128.     // This should be combined with your Start function. See Fig 2.4
129.     void Awake ()
130.     {
131.         CharacterController controller = GetComponent<CharacterController>();
132.         if(!controller)
133.             gameObject.AddComponent ("CharacterController");
134.     }
135.  
136.     public void AddImpact(Vector3 dir, float force)
137.     {
138.         dir.Normalize();
139.         if (dir.y < 0)
140.             dir.y = -dir.y; // reflect down force on the ground
141.         impact += dir.normalized * force / mass;
142.     }
143.  
144. }

Refactored Code:

Code (CSharp):

1. using UnityEngine;
2. using System.Collections;
3.  
4. /* Figure 1
5. * Having helper classes that can be used in other objects can be
6. * beneficial! This is a simple example.
7. */
8. public class Timer {
9.     float timeFinished;
10.     float lastAmount;
11.  
12.     public bool expired {
13.         get {
14.             return (timeFinished >= Time.time);
15.         }
16.     }
17.  
18.     public float remaining {
19.         get {
20.             if (expired) {
21.                 return 0f;
22.             } else {
23.                 return timeFinished - Time.time;
24.             }
25.         }
26.     }
27.  
28.     public Timer (float amount) {
29.         Set(amount);
30.     }
31.  
32.     public void Set(float amount) {
33.         if (amount <= 0f) {
34.             throw new System.Exception("Timers cannot be set with negative amounts.");
35.         }
36.  
37.         timeFinished = Time.time + amount;
38.  
39.         lastAmount = amount;
40.     }
41.  
42.     public void Reset(){
43.         Set (lastAmount);
44.     }
45.  
46. }
47.  
48. /* Fig 2.1: Universal Constants
49. *
50. * I wouldn't put constants for a particular object that are applicable to many objects in a single objects class.
51. * Now, I don't know how your game works, maybe every object is supposed to have it's own gravity, but tyically
52. * I would put all my world constants in a static class and then reference them from there.
53. *
54. * Like so: */
55. public static class Game {
56.  
57.     //I also imagine gravity would generally be constant.
58.     public const float Gravity = 25f;
59.  
60.     public static class Buttons {
61.         public const string Jump = "Jump";
62.         public const string Dash = "Dash";
63.     }
64.  
65. }
66.  
67. public class Example : MonoBehaviour {
68.  
69.     // Fig 2.2 grouped fields
70.     // Makes this class viewable in the editor
71.     [System.Serializable]
72.     public class Settings {
73.         // In the unity editor, there will be a slider for these ranges
74.         [Range(1f,10f)]
75.         public float speed = 6f;
76.  
77.         [Range(1f,10f)]
78.         public float jumpSpeed = 8f;
79.  
80.         [Range(10f,40f)]
81.         public float boostForce = 18f;
82.  
83.         [Range(10f,40f)]
84.         public float dashForceGround = 18f;
85.  
86.         [Range(10f,40f)]
87.         public float dashForceAir = 33f;
88.  
89.         [Range(0f,1f)]
90.         public float dashTimeDelay = 0.7f;
91.  
92.         //Mass wouldn't change, but outside code would be able to look at it
93.         public const float Mass = 1f;
94.  
95.         public const float ImpactMovementThreshold = 0.2f;
96.  
97.         public const float ImpactEnergyAbsorption = 5f;
98.  
99.     }
100.     public Settings settings = new Settings();
101.  
102.     // Fig 2.3 read-only properties
103.     bool _isGrounded = true;
104.     public bool isGrounded {
105.         get {
106.             return _isGrounded;
107.         }
108.     }
109.  
110.     bool _usedBoost = false;
111.     public bool usedBoost {
112.         get {
113.             return _usedBoost;
114.         }
115.     }
116.  
117.     Vector3 impact = Vector3.zero;
118.     Vector3 moveDirection = Vector3.zero;
119.  
120.     Timer dashTimer;
121.  
122.     CharacterController controller;
123.  
124.     //regions are another way to help organize code
125.     #region main
126.  
127.     void Awake(){
128.         //Fig 2.4 Combined intializations
129.         controller = GetComponent <CharacterController> ();
130.         if (!controller) {
131.             controller = gameObject.AddComponent <CharacterController>();
132.         }
133.  
134.     }
135.  
136.     void Start() {
137.  
138.         //dashTimer set once Behaviour created and enabled
139.         dashTimer = new Timer(settings.dashTimeDelay);
140.    
141.     }
142.  
143.     void Update () {
144.  
145.         //Fig 3.1 Organized Update Loop
146.         DetermineGrounded();
147.         ApplyUserInput();
148.         ApplyForces();
149.    
150.     }
151.  
152.     #endregion
153.  
154.     #region update loop
155.  
156.     void DetermineGrounded(){
157.  
158.         CollisionFlags flags = controller.Move (moveDirection * Time.deltaTime);
159.         _isGrounded = (flags & CollisionFlags.CollidedBelow) != 0;
160.  
161.     }
162.  
163.     void ApplyUserInput(){
164.  
165.         var horizontal = Input.GetAxis("Horizontal");
166.         var vertical = Input.GetAxis("Vertical");
167.  
168.         Vector3 inputVector = new Vector3(horizontal, 0f, vertical);
169.  
170.         if (isGrounded) {
171.  
172.             moveDirection = transform.TransformDirection(inputVector) * settings.speed;
173.  
174.             if (Input.GetButtonDown(Game.Buttons.Jump)) {
175.                 moveDirection.y = settings.jumpSpeed;
176.             }
177.  
178.             _usedBoost = false;
179.  
180.         //Fig 3.2 combined redundant conditions.
181.         //We'll only get here if we're not grounded.
182.        } else if (Input.GetButtonDown(Game.Buttons.Jump) && !_usedBoost) {
183.  
184.             //Fig 3.3 boost is combined into this function
185.             moveDirection.y = settings.boostForce;
186.             _usedBoost = true;
187.  
188.         }
189.  
190.         //Fig 3.4 dash validation.
191.         //If it's grounded or you're not moving forward, a dash is valid.
192.         bool dashValid = isGrounded || vertical < 0f;
193.  
194.         //Fig 3.5 combined dash into this function
195.         if (Input.GetButtonDown(Game.Buttons.Dash) && dashTimer.expired && dashValid) {
196.  
197.             //Fig 3.6
198.             //Cleaned up a bit for less redundancy. Notice the inclusion of the
199.             //inputVector at the top.
200.             float dashForce = (isGrounded) ? settings.dashForceGround : settings.dashForceAir;
201.             moveDirection = transform.TransformDirection(inputVector);
202.             AddImpact(moveDirection, dashForce);
203.  
204.             dashTimer.Reset();
205.  
206.         }
207.  
208.     }
209.  
210.     void ApplyForces(){
211.  
212.         if (impact.magnitude > Settings.ImpactMovementThreshold) {
213.  
214.             controller.Move(impact * Time.deltaTime);
215.    
216.         }
217.  
218.         impact = Vector3.Lerp(impact, Vector3.zero, Settings.ImpactEnergyAbsorption * Time.deltaTime);
219.  
220.         //Fig 4 only apply gravity EDIT: when NOT gounded.
221.         if (!isGrounded) {
222.  
223.             moveDirection.y -= Game.Gravity * Time.deltaTime;
224.  
225.         }
226.     }
227.  
228.     #endregion
229.  
230.     #region helper functions
231.  
232.     public void AddImpact(Vector3 dir, float force)
233.     {
234.    
235.         dir.Normalize();
236.         if (dir.y < 0)
237.             dir.y = -dir.y;
238.         impact += dir.normalized * force / Settings.Mass;
239.  
240.     }
241.  
242.     #endregion
243. }
244.  

 

Damn man, what a reply!
Thanks so much this is super helpful, and I very much appreciate the effort spent, honestly. Fantastic, thanks so much. I have been coding for a while now but was 'self-taught' in that I learned it all through learn c++ books and then trial&error with a few XNA titles, so although I am confident that I can hash out code to do what I want, sometimes it may not be the best approach, and never really struck me until I put up some tutorials and people were ripping on my code, so I figured I would focus on improving in that area. This definitely helps, awesome. Again thanks.

 

Just wanted to say that this is a fantastic reply.

 

Consider dropping obvious comments. Comments should be used to enhance the codes, by pointing out intent or reasoning. They should not be used to restate what the code does. Assume any user of your script knows how to read C#. As your project gets bigger you will find maintaining the level and accuracy of comments difficult. Comments that don't match the code are worse then useless.

There is some divided opinion on this practice.