CODEBUSTERS: Bust My Code

The rules are simple - review the snippet of code in the post above yours, and then paste a snippet of your own code for review by your fellow programmers of the Unity forums. This is a fun little exercise and a great way to learn crazy wicked cool new ways of writing code and solving problems.

Rules:
1. Use code tags!

Code (csharp):

1.  public float thisIsHowWeRoll = Mathf.Infinity;

2. Post what your code is supposed to do
It's hard to verify that it's functional if people don't know what the function is supposed to be

3. Keep feedback constructive
Everyone knows that curly braces should start on a newline, but you're not a terrible programmer if you choose to keep them on the same line (just a terrible person ). Post feedback related to logic flow, not stylistic choices.

4. Post Code to Review Code
Make sure and post a snippet of your own code for review when you review someone else's - CODEBUSTERS is a non-spectator sport.

3… 2… 1… Go!

 

I'll kick it off. This is a terrain script ripped out of an infinite sidescroller we're working on:

Code (csharp):

1.  
2. using UnityEngine;
3. using System;
4. using System.Collections;
5. using System.Collections.Generic;
6. using System.Linq;
7.  
8. //We assume that terrain blocks pivot at the left side, FYI
9. public class Terrain : MonoBehaviour {
10.     public TruckMovement truckMovement;
11.     public Transform[] terrainBlocks;
12.  
13.     public float screenEdgePadding = 20;    //how much we compensate for the perspective
14.     public float terrainBlockWidth = 40;
15.  
16.     float _currentTerrainX;            //this is the point at which we will spawn the next terrain block
17.  
18.     void Start(){
19.         for (var i = 0; i < terrainBlocks.Length; i++)
20.             terrainBlocks[i].position = new Vector3 (i * terrainBlockWidth, terrainBlocks[i].position.y, terrainBlocks[i].position.z);
21.        
22.         InitialSetup();
23.     }
24.  
25.     [ContextMenu("Execute Initial")]
26.     void InitialSetup(){
27.         foreach(var curBlock in terrainBlocks)
28.             curBlock.transform.position = Vector3.right * (TerrainLeft - terrainBlockWidth - 1);    //shove all the blocks off screen
29.  
30.         _currentTerrainX = TerrainLeft;
31.         while(NeedsBlockInFront()){
32.             PlaceNextBlock(NextTerrainBlock());
33.         }
34.     }
35.  
36.     void Update(){
37.         while(NeedsBlockInFront()){
38.             PlaceNextBlock(NextTerrainBlock());
39.         }
40.     }
41.  
42.     public void PlaceNextBlock(Transform block){
43.         block.transform.localPosition = Vector3.right * _currentTerrainX;
44.         _currentTerrainX += terrainBlockWidth;        //TODO: make this support arbitrary block lengths
45.     }
46.  
47.     /// <summary>
48.     /// Determines if we need to catch up
49.     /// </summary>
50.     /// <returns><c>true</c>, if block in front was needsed, <c>false</c> otherwise.</returns>
51.     public bool NeedsBlockInFront(){
52.         //TODO: compensate for blocks being visible past the gameplay edge (in distance perspective)
53.         return _currentTerrainX < TerrainRight;  
54.     }
55.  
56.     /// <summary>
57.     /// Determines if a block has been left too far behind to be seen anymore
58.     /// </summary>
59.     public bool BlockHasFallenOff(Transform block){
60.         //TODO: we need to handle different sized blocks
61.         return (block.transform.position.x < TerrainLeft);      
62.     }
63.  
64.     public float TerrainLeft{
65.         get {
66.             return GameScreen.Left - terrainBlockWidth - screenEdgePadding;
67.         }
68.     }
69.  
70.     public float TerrainRight{
71.         get{
72.             return GameScreen.Right + screenEdgePadding;
73.         }
74.     }
75.  
76.     /// <summary>
77.     /// Calculates which block should go next in the generated terrain
78.     /// </summary>
79.     /// <returns>The terrain block.</returns>
80.     public Transform NextTerrainBlock(){
81.         //TODO: handle different terrain styles
82.         //TODO: get rid of linq
83.         return terrainBlocks.First(x => BlockHasFallenOff(x));    //TODO: IF A GAMEOBJECT IS INACTIVE, It can be used
84.     }
85. }
86.  

The purpose of this code is to take a list of GameObjects of equal length and lay them out in a horizontal pattern. When the camera moves past the right edge of the list of objects, we pull one off the left edge and place it in front.

 

Busted!

 

Well, you could have fixed that in less time than it took to write that TODO..but then you wouldn't have as many comments..amirite ?

 

For the most part looks good, maybe a little convoluted in places: on Start() you move all the blocks, then you call InitialPosition which moves all the blocks then you call place next block which moves some of the blocks.

You also might want to consider moving some of the functions to a block class, although it would only be worthwhile if things are likely to get more complex.

Suppose I best post something to keep in the spirit of initial post.

Code (csharp):

1.  
2. var damageLevels:int = 0;
3. var damageForce:float = 5.0;
4. var emitter:ParticleEmitter;
5. var textureOffset:float = 0.25f;
6.  
7. private var currentDamage:int;
8. private var mass:float;
9.  
10. static var BOUNCE_FORCE : float = -0.33;
11.  
12. function OnCollisionEnter (collision:Collision) {
13.     if (GetAppliedForce(collision, rigidbody) >= damageForce) {
14.             DoDamage(GetAppliedForce(collision, rigidbody), collision.rigidbody);
15.     }
16. }
17.  
18. function DoDamage(f:float, r:Rigidbody) {
19.     if ((currentDamage + Mathf.Floor(f / damageForce)) < (damageLevels + 1)) {
20.         renderer.material.mainTextureOffset.y -= textureOffset * Mathf.Floor(f / damageForce);
21.         currentDamage += Mathf.Floor(f / damageForce);
22.     } else {
23.         if (emitter != null) emitter.emit = true;
24.         if (r != null) r.AddForce(r.velocity * BOUNCE_FORCE, ForceMode.VelocityChange);
25.         gameObject.active = false;
26.     }
27. }
28.  
29. function GetAppliedForce(c:Collision, r:Rigidbody):float {
30.     if (c.rigidbody != null) return ((r.velocity * r.mass) - (c.rigidbody.velocity * c.rigidbody.mass)).magnitude;
31.     return (r.velocity * r.mass).magnitude;
32. }
33.  

Handles cycling damage texture + particles like this:

This is pretty old, haven't been using UnityScript much lately, although this kind of makes me want to do so

 

Your variable names are much nicer, but this was Unity 3 so I'm not sure SetActive was available back then. Multiple calls could certainly be wrapped in temp variables, this is one thing I fail to do quite often even when writing "serious code". I'm not too concerned about the performance in this case, but it would likely make the code easier to understand.

I'll leave it to someone else to analyse your code, I don't want to close the loop.

 

You should really be storing transform in a member variable.
And, transform.position should be stored in a variable too at the top of GenerateBridge()

 

This is a piece I have written almost a year ago. I always use to make a health system that darkens your screen with some blood texture

Code (csharp):

1.  
2. using UnityEngine;
3. using System.Collections;
4.  
5. public class PlayerHealth : MonoBehaviour
6. {
7.     public float currentHealth;                 //The current amount of health we have
8.     public float maxHealth;                     //The maximum amount of health to have
9.     public float healthRegenTime;               //Time before health starts to regenerate
10.     public float healthRegenAmount;             //The amount of health to regenerate per second
11.     public Texture2D dmgTexture;                //The texture that will act as the damage indicator
12.  
13.     private float untilRegen;                   //The time until we can start regenerating health
14.     private Rect rectZero;                      //Used to hold the position and scale of the damage texure
15.  
16.     void Start()
17.     {
18.         //Make sure the player starts at full health
19.         currentHealth = maxHealth;
20.         //Make the Rect in the zero position, with the height and width the same as the screen
21.         rectZero = new Rect(0, 0, Screen.width, Screen.height);
22.     }
23.  
24.     void Update()
25.     {
26.         //Regenerate health
27.         HealthRegen();
28.     }
29.  
30.     void OnGUI()
31.     {
32.         Color newColor = GUI.color;                 //Used to hold the new color value of the texture
33.         newColor.a = GetHealthPercentage();         //Set the new alpha to the texture according to the current health
34.  
35.         //Set the color of the texture to the updated one
36.         GUI.color = newColor;
37.         //Draw the texture on the screen so that it fits the screen completely
38.         GUI.DrawTexture(rectZero, dmgTexture, ScaleMode.StretchToFill);
39.     }
40.  
41.     void HealthRegen()
42.     {
43.         //If the current health is less than maximum health and the regen timer is over...
44.         if (currentHealth < maxHealth && Time.time > untilRegen)
45.         {
46.             //Add 1 health to the current health every second
47.             currentHealth += healthRegenAmount * Time.deltaTime;
48.  
49.             //Make sure the current health doesnt exceed the maximum health
50.             if (currentHealth > maxHealth)
51.                 currentHealth = maxHealth;
52.         }
53.     }
54.  
55.     void ApplyDamage(float damageToApply)
56.     {
57.         //Subtract the damage amount to the current health
58.         currentHealth -= damageToApply;
59.  
60.         //If we have less than 0 health then end the game
61.         if (currentHealth <= 0f)
62.             return; //end Game ==============================================================================================
63.  
64.         //Start the health regen timer
65.         untilRegen = Time.time + healthRegenTime;
66.     }
67.  
68.     float GetHealthPercentage()
69.     {
70.         //Calculate the percentage of health left and return the value
71.         float newValue = currentHealth / maxHealth;
72.         newValue = 1 - newValue;
73.         return newValue;
74.     }
75. }
76.  
77.  

Edit: This is code that I have written solely by myself. If you perhaps would like to use this code, please feel free to do so as I see a lot of people asking how to do blood health screens. Thanks!

 

From a technical perspective, this looks pretty well done - nice clean code and does what it says on the tin (bonus points for gizmos!). Of note: when I ran it, all of the rigidbodies were asleep on Start, so the bridge didn't fall - I assume we've got different physics settings.

From a functional perspective, I'd prefer something with a bit more flexibility in terms of control - mostly a way of defining how far apart the bridge pieces are (without that, there's not really a good way to control the sag).

Not really a point in this case - accessing transform is not terribly slow, and the bridge generation is only run once.

 

Yep, I rarely if ever optimise my run-once code.

 

Nice!
You seem to have a pretty good grasp of the language, which is good. I'd recommend thinking through your program structure - it sends up red flags when you have a single class controlling a GUI item(blood screen), player health, and win condition all at the same time. If you were to rewrite it, I'd recommend splitting it up into:
1. A health script that handles taking damage and regeneration
2. A blood screen script that fades your texture in and out based on a health script (it could be the player's health, or even an NPC's health).
3. A game script that handles the win condition - i.e ends the game when a certain Health's currentDamage drops below zero.


So this script is from years ago (back when I used US… I kind of miss it in a sick way) designed to cycle through a sprite atlas to produce a sprite animation.

Code (csharp):

1.  
2. #pragma strict
3.  
4. var atlas : Texture2D;
5. var uvs : Rect[];
6. var fps = 24;
7. var loop = false;
8. var playOnStart = true;
9. var isPlaying = false;
10.  
11.  
12. private var _currentFrame = 0;
13.  
14. function Start()
15. {
16.  if(playOnStart) Play();
17. }
18.  
19. function Play() : IEnumerator
20. {
21.  renderer.enabled = true;
22.  isPlaying = true;
23.  _currentFrame = 0;
24.  
25.  while(_currentFrame < uvs.length)
26.  {
27.  var curUV = uvs[_currentFrame];
28.  renderer.material.mainTexture = atlas;
29.  renderer.material.mainTextureOffset = Vector2(curUV.x, curUV.y);
30.  renderer.material.mainTextureScale = Vector2(curUV.width, curUV.height);
31.  yield WaitForSeconds(1.0 / (fps + 0.0));
32.  _currentFrame ++;
33.  }
34.  
35.  if(loop) Play();
36.  else Stop();
37. }
38.  
39. function Stop()
40. {
41.  isPlaying = false;
42.  StopAllCoroutines();
43.  renderer.enabled = false;
44. }
45.  

 

Thanks. I didn't know people usually split scripts up that much. Is that what AAA studios usually do? Thanks for the suggestions too.

Also, I also noticed that in your code, you are not changing the var loop at all within the script. I assume you are controlling it in another script? It's a good thing we got U4.2 so we don't have to worry about sprites anymore huh haha

 

I just wanted to compliment you for the neatness of your code. It's people like you that makes life easier

One thing that annoyed me was that you lacked comments.... maybe you just took them out or something but keep comments gets rid of a lot of speculation / questions. Anyways, code looks nice. One question, this code is only run once. That means that you are not going to use those bridge pieces again in another script? Because I don't see a reference to some sort of pooling system or anything. Possibly there is no level scrolling or there is only one bridge at the beginning of the level?

 

When using component oriented design, yes, it's generally considered good practice to keep each component to having one role or responsibility. Think about, for instance, if you later decided that you wanted stuff other than health to control the screen image?

Also, many or verbose comments aren't always a good thing. There's a school of thought that you should have a minimum number of comments in favor of "self documenting code", with the argument being that comments themselves need maintaining and can be 'buggy', and an incorrect comment can cause far more damage than not having a comment. Comments should generally explain why you're doing things, not what you're doing. In a quick skim of that code there's nothing that I don't think I grasped immediately, so comments could be an overhead without a benefit.

 

Not in this case, but in general it will make your code slightly faster, and if we're picking apart code here, may as well throw all the optimisations in

 

@flaminghairball I wouldn't consider myself anything more than a novice when it comes to programming so please take this with a grain or three of salt. But when reading over your animation snippet I didn't see anything that jumped out as needing refactored.

The only thing that really caught my eye, is the disabling of the renderer as a whole when the animation was complete. I would feel safe to assume that this is hooked into a controller which would probably choose the next animation set after this was finished, but couldn't this cause a frame or two where nothing is rendered?

@superpig A bit out of order, but has been eating at me with your snippet and I was just curious as to your decision to due so - On line 50, you dynamically type for the HingeJoint2D when you've been static typing the rest of the class. Was there any conscious reason behind this? Again, I am just curious.

Had to do a bit of digging to find something that didn't have it's teeth hooked into several other components, so this is from a fairly dated project and is probably due for its share of refactoring.

Code (csharp):

1.  
2. using UnityEngine;
3. using System.Collections.Generic;
4.  
5. public class GravitationalBody : MonoBehaviour
6. {
7.     //-------------------------------------------------------------------
8.     #region Fields
9.  
10.     [SerializeField]
11.     private float m_Range = 25.0f;
12.     [SerializeField]
13.     private float m_ForceMultiplyer = 2.0f;
14.     [SerializeField]
15.     private float m_RotationDamping = 1.5f;
16.  
17.     private List<Rigidbody> m_EffectedBodies = new List<Rigidbody>();
18.     private Collider[] m_CollidersInRange;
19.  
20.     #endregion
21.  
22.  
23.     //-------------------------------------------------------------------
24.     #region FixedUpdate method
25.  
26.     private void FixedUpdate()
27.     {
28.         Rigidbody rb;
29.         this.m_EffectedBodies.Clear();
30.         this.m_CollidersInRange = Physics.OverlapSphere(this.transform.position, this.m_Range);
31.      
32.         for (int i = 0; i < this.m_CollidersInRange.Length; i++)
33.         {
34.             if (this.m_CollidersInRange[i].rigidbody == null)
35.             {
36.                 continue;
37.             }
38.  
39.             rb = this.m_CollidersInRange[i].attachedRigidbody;
40.             if(rb == this.rigidbody || AffectedRigidbodies.Contains(rb) == true)
41.             {
42.                 continue;
43.             }
44.  
45.             this.m_EffectedBodies.Add(rb);
46.  
47.             Vector3 offset = this.transform.position - this.m_CollidersInRange[i].transform.position;
48.             rb.AddForce(offset / offset.sqrMagnitude * this.rigidbody.mass * this.m_ForceMultiplyer);
49.  
50.             if (this.m_CollidersInRange[i].gameObject.rigidbody.freezeRotation == true)
51.             {
52.                 Vector3 globalUpwards = this.m_CollidersInRange[i].gameObject.transform.position - this.transform.position;
53.                 globalUpwards.Normalize();
54.                 Vector3 localUpwards = this.m_CollidersInRange[i].gameObject.transform.up;
55.  
56.                 Quaternion angle = Quaternion.FromToRotation(localUpwards, globalUpwards);
57.                 angle *= this.m_CollidersInRange[i].transform.rotation;
58.                 this.m_CollidersInRange[i].transform.rotation = Quaternion.Slerp(this.m_CollidersInRange[i].transform.rotation, angle, this.m_RotationDamping);
59.             }
60.         }
61.     }
62.  
63.     #endregion
64. }
65.  

 

What are the benefits you find from using var over a static typing? I try to avoid using it as I think it increases readability to see the typing up front, but that's only my preference.

Ah yes, this was an awkward time in my life where I was torn between trying to carry over some prefixes from my short lived C++ life and couldn't decide between the this. prefix and the ugly notations I used when toying with Irrlicht and other rendering libraries some years ago. Trust me, I had some counseling and this has been resolved

I do apologize for the slip on line 40, I pulled this from an old SVN repo and assumed that since it was pushed it must have been valid; not halfway between refactoring. The line should read as m_EffectedBodies. I haven't looked into the HashSet before, and to be completely honest haven't even heard of it - though now I feel I should.

These are excellent points indeed, and I have seen other projects and assets do things as described here. If I ever revisit this prototype I will definitely be taking this into consideration. Reading back over the class, I can agree about the natural readability of * == true comparisons. I am sure at the time I did the comparisons to signal what the return value of the method would be, but on the other hand it is made obvious by the if statement.

I apologize that I can't be of more assistance regarding your latest code posting, but aside from the SetActive which you've already acknowledged, it reads as a clean piece of code to me.

If no one is opposed, here is another from me. The class is the controller for a bomb object within a networked prototype, whose purpose is to handle creation and destruction of the bomb object while syncing rigidbodies and distributing messages to the clients of who was damaged in the process.

Having never worked in a team with another programmer, I really appreciate what and where this thread is going with allowing peer review of each others code. I really hope this thread goes the distance!

Code (csharp):

1.  
2. using UnityEngine;
3. using System.Collections;
4.  
5. public class BombController : TNBehaviour
6. {
7.      //--------------------------------------------------------------
8.     #region Fields
9.  
10.     [SerializeField]
11.     private Vector3 InitialForce = Vector3.up;
12.      [SerializeField]
13.      private float Lifespan = 7.5f;
14.     [SerializeField]
15.     private LayerMask EffectedLayers;
16.     [SerializeField]
17.     private float Range = 5.0f;
18.     [SerializeField]
19.     private float DamageMultiplier = 2.5f;
20.     [SerializeField]
21.     private GameObject ExplosionPrefab;
22.  
23.      private bool IsMine = false;
24.  
25.      #endregion
26.  
27.  
28.         //--------------------------------------------------------------
29.         #region Properties
30.  
31.         public bool Mine
32.         {
33.             get
34.             {
35.                 return this.IsMine;
36.             }
37.         }
38.  
39.         #endregion
40.  
41.  
42.         //--------------------------------------------------------------
43.         #region Awake method
44.  
45.         private void Awake()
46.         {
47.             this.IsMine = TNManager.isThisMyObject;
48.         }
49.  
50.         #endregion
51.  
52.  
53.     //--------------------------------------------------------------
54.     #region Start method
55.  
56.     private void Start()
57.     {
58.         if (this.IsMine == true)
59.         {
60.             this.rigidbody.AddForce(this.InitialForce, ForceMode.Impulse);
61.             StartCoroutine(this.Countdown());
62.         }
63.     }
64.  
65.     #endregion
66.  
67.  
68.     //--------------------------------------------------------------
69.     #region Countdown coroutine
70.  
71.     private IEnumerator Countdown()
72.     {
73.         yield return new WaitForSeconds(this.Lifespan);
74.  
75.             if (this.IsMine == true)
76.             {
77.                 Collider[] effected = Physics.OverlapSphere(this.transform.position, this.Range, this.EffectedLayers);
78.                 for (int i = 0; i < effected.Length; i++)
79.                 {
80.                     if(effected[i].gameObject.tag != "Player" && effected[i].gameObject.tag != "Bomb")
81.                     {
82.                         if(effected[i].rigidbody != null)
83.                         {
84.                             effected[i].rigidbody.AddExplosionForce(7.5f, this.transform.position, 3.5f, 0.75f, ForceMode.Impulse);
85.                             effected[i].GetComponent<TNSyncRigidbody>().Sync();
86.                         }
87.  
88.                         continue;
89.                     }
90.  
91.                     float proximity = (this.transform.position - effected [i].transform.position).magnitude;
92.                     float falloff = 2 - (proximity / this.Range);
93.  
94.                     TNObject obj = effected[i].GetComponent<TNObject>();
95.                     if (obj != null)
96.                     {
97.                         Messenger<int, float , Vector3>.Broadcast("OnRequestDamage", obj.ownerID, (this.DamageMultiplier * falloff), this.transform.position);
98.                     }
99.                 }
100.             }
101.  
102.             TNManager.Create(this.ExplosionPrefab, this.transform.position, Quaternion.identity);
103.             TNManager.Destroy(this.gameObject);
104.     }
105.  
106.     #endregion
107. }
108.  

EDIT - I apologize for the formatting, Atom seems have had a heyday with my tab spacing settings.

 

Yetch. All this code here is awful - sorry. Just pick up a book on writing decent code (and then probably forget it all because Unity probably won't let you do it that way). Something like this: http://www.amazon.com/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882

 

The worst thing you can do to a programmer morally is call their code bad..... well unless it really is bad code. May we criticize your code?

Anyways @superpig how come you don't use static variables? I think using var is a bad practice because
1. You could unintentionally have Vector3 but you wanted a float for example
2. Its make code harder to read, especially when you are constantly looking back at variables
3. *I heard* it takes the compiler -slightly- more time to run code cause it has to figure out the type

Am I missing something about the benefit of using var? I am just curious on why everyone likes to use them

 

Never heard of that "worst" thing before. Anyway - no you may not see my code because my code is sadly not available for the GP. Honestly just read a good book on how to write clearly. Its not rocket science.

In regards to your other point I agree strongly! Var is misused so often - as you highlighted in superpigs case.

 

I wonder if this bit of code is really as bad as I think?

Code (csharp):

1.  
2. using UnityEngine;
3. using System.Collections;
4.  
5. public class GameScript : MonoBehaviour {
6.  
7. public GameObject starryThingPrefab;public GameObject magnetThingPrefab;
8.  
9. public GameObject minibossBannerPrefab;
10.  
11. private float timeElapsed = 0;
12.  
13. private bool encounter1triggered = false;
14.  
15. private bool encounter2triggered = false;private float encounter2time = 0.0f;
16.  
17. private bool encounter3triggered = false;private float encounter3time = 0.0f;
18.  
19. private bool minibossBannerTriggered = false;private float minibossBannerTime = 0.0f;
20.  
21. private bool minibossTriggered = false;
22.  
23. private bool encounter1Atriggered = false;private float encounter1Atime = 0.0f;
24.  
25. private bool encounter2Atriggered = false;private float encounter2Atime = 0.0f;
26.  
27. private bool encounter3Atriggered = false;private float encounter3Atime = 0.0f;
28.  
29. private bool finalBossTriggered = false;
30.  
31. private bool scriptFinished = false;
32.  
33. public void Update(){
34.  
35. if (scriptFinished) return;
36.  
37. timeElapsed += Time.deltaTime;if (!encounter1triggered){TriggerEncounter1();return;}
38.  
39. if (!encounter2triggered){if (timeElapsed >= 5){TriggerEncounter2();}return;}
40.  
41. if (!encounter3triggered){if (GameObject.FindObjectOfType<Enemy>() == null) // no more enemies!{TriggerEncounter3();}return;}
42.  
43. if (!minibossBannerTriggered){if (GameObject.FindObjectOfType<Enemy>() == null) // no more enemies!{Instantiate(minibossBannerPrefab);minibossBannerTriggered = true;minibossBannerTime = timeElapsed;}return;}
44.  
45. if (!minibossTriggered){if(timeElapsed > minibossBannerTime + 5) // no more enemies!{TriggerMiniboss();}return;}
46.  
47. if (!encounter1Atriggered){if (GameObject.FindObjectOfType<Enemy>() == null) // no more enemies!{TriggerEncounter1A();}return;}if (!encounter2Atriggered){if (timeElapsed > encounter1Atime + 5){TriggerEncounter2A();}return;}if (!encounter3Atriggered){if (timeElapsed > encounter1Atime + 10)
48. {
49. TriggerEncounter3A();
50. }
51. }
52.  
53. // check if game is finished
54. if (GameObject.FindObjectOfType<Enemy>() == null) // no more enemies!
55. {
56. GameObject.FindObjectOfType<PlayerShip>().Victory();
57. scriptFinished = true;
58. }
59. }
60.  
61. private void TriggerEncounter1()
62. {
63. encounter1triggered = true;
64. GameObject enemyA = (GameObject)Instantiate(starryThingPrefab);
65. GameObject enemyB = (GameObject)Instantiate(starryThingPrefab);
66.  
67. enemyA.transform.position = new Vector3(3.5f, -7, 0);
68. enemyA.GetComponent<StarryThing>().targetPosition = new Vector2(3, -2);
69. enemyA.GetComponent<StarryThing>().rotationSpeed = 0;
70. enemyA.GetComponent<StarryThing>().firingCooldownInit = 0.5f;
71.  
72. enemyB.transform.position = new Vector3(3.5f, 7, 0);
73. enemyB.GetComponent<StarryThing>().targetPosition = new Vector2(3, 2);
74. enemyB.GetComponent<StarryThing>().rotationSpeed = 0;
75. enemyB.GetComponent<StarryThing>().firingCooldownInit = 0.5f;
76. }
77.  
78. private void TriggerEncounter2()
79. {
80. encounter2triggered = true;
81. encounter2time = timeElapsed;
82. GameObject enemyA = (GameObject)Instantiate(starryThingPrefab);
83. GameObject enemyB = (GameObject)Instantiate(starryThingPrefab);
84.  
85. enemyA.transform.position = new Vector3(3.5f, -7, 0);
86. enemyA.GetComponent<StarryThing>().targetPosition = new Vector2(1, -2);
87.  
88. enemyB.transform.position = new Vector3(3.5f, 7, 0);
89. enemyB.GetComponent<StarryThing>().targetPosition = new Vector2(1, 2);
90. }
91.  
92. private void TriggerEncounter3()
93. {
94. encounter3triggered = true;
95. encounter3time = timeElapsed;
96.  
97. GameObject enemyA = (GameObject)Instantiate(starryThingPrefab);
98. GameObject enemyB = (GameObject)Instantiate(starryThingPrefab);
99.  
100. enemyA.transform.position = new Vector3(3.5f, -7, 0);
101. enemyA.GetComponent<StarryThing>().targetPosition = new Vector2(1, -2);
102.  
103. enemyB.transform.position = new Vector3(3.5f, 7, 0);
104. enemyB.GetComponent<StarryThing>().targetPosition = new Vector2(1, 2);
105.  
106. GameObject enemyC = (GameObject)Instantiate(magnetThingPrefab);
107. enemyC.transform.position = new Vector3(8, 0, 0);
108. enemyC.GetComponent<MagnetThing>().targetPosition = new Vector2(3, 0);
109. }
110.  
111. private void TriggerMiniboss()
112. {
113. minibossTriggered = true;
114. GameObject miniboss = (GameObject)Instantiate(starryThingPrefab);
115. miniboss.GetComponent<StarryThing>().targetPosition = new Vector2(3, 2);
116. miniboss.transform.localScale = new Vector3(0.3f, 0.3f, 0.3f);
117. miniboss.GetComponent<StarryThing>().rotationSpeed = 45;
118. miniboss.GetComponent<StarryThing>().firingCooldownInit = 0.05f;
119. miniboss.GetComponent<StarryThing>().hitpoints = 10;
120. miniboss.GetComponent<StarryThing>().bulletSpeed = 3;
121. }
122.  
123. private void TriggerEncounter1A()
124. {
125. encounter1Atriggered = true;
126. encounter1Atime = timeElapsed;
127.  
128. GameObject enemyA = (GameObject)Instantiate(magnetThingPrefab);
129. enemyA.transform.position =  new Vector3(3.5f, -7, 0);
130. enemyA.GetComponent<MagnetThing>().targetPosition = new Vector2(3.5f, 0);
131. }
132.  
133. private void TriggerEncounter2A()
134. {
135. encounter2Atriggered = true;
136. encounter2Atime = timeElapsed;
137.  
138. GameObject enemyA = (GameObject)Instantiate(magnetThingPrefab);
139. enemyA.transform.position = new Vector3(3.5f, -7, 0);
140. enemyA.GetComponent<MagnetThing>().targetPosition = new Vector2(2f, -2);
141.  
142. GameObject enemyB = (GameObject)Instantiate(magnetThingPrefab);
143. enemyB.transform.position = new Vector3(3.5f, 7, 0);
144. enemyB.GetComponent<MagnetThing>().targetPosition = new Vector2(2f, 2);
145. }
146.  
147. private void TriggerEncounter3A()
148. {
149. encounter3Atriggered = true;
150. GameObject enemyA = (GameObject)Instantiate(magnetThingPrefab);
151. enemyA.transform.position = new Vector3(3.5f, -7, 0);
152. enemyA.GetComponent<MagnetThing>().targetPosition = new Vector2(4f, -2);
153.  
154. GameObject enemyB = (GameObject)Instantiate(magnetThingPrefab);
155. enemyB.transform.position = new Vector3(3.5f, 7, 0);
156. enemyB.GetComponent<MagnetThing>().targetPosition = new Vector2(4f, 2);
157. }
158. }
159.  

It's the enemy spawning script for my game, it spawns enemies in waves/encounters, each wave/encounter is one function.

(edit: Copy&Paste ate my indention... )

 

I use them purely for a readability aspect, I find it makes my code 'easier' to read.
I don't use it for value types, I use it only for declaring reference types (classes etc)

For instance, instead of having

Code (csharp):

1. List<MyAwesomeClass> myList = new List<MyAwesomeClass>();

using var is much cleaner and easier to read

Code (csharp):

1. var myList = new List<MyAwesomeClass>();

Why do you need to specify the declaration twice in the same line?

I have seen people use var for value types, i.e int, string etc, and I disagree with this, it makes code harder to understand and read.

Code (csharp):

1.  
2. var x = 0;
3. var f = 0.0f;
4.  
5. x = x + 1;
6. f = f / x;
7.  

In this case I always declare the variable type explicitly as I don't see any benefit var brings here..

 

Yep, in Unity 5 Beta 1 it was implemented

 

Ah I understand better. I really like this thread. Learning a lot so far

 

Ooh yeah, both of these are really neat ideas! I really don't see the full potential of Unity's tools way too often.

Thanks a lot.

 

This is my favourite way to look for child objects...

If you have

GameObject
- Child
- - GrandChild
- - - GreatGrandChild

And you want to get a reference to GreatGrandChild from GameObject.
You could simply use...

Code (csharp):

1. var greatGrandChildObject = transform.Find("Child/GrandChild/GreatGrandChild");

Nice and clean

 

Instead of a bunch of ifs and returns in your update loop, you can create a queue of actions like this:

Code (csharp):

1. private Queue<Func<bool>> actionQueue;
2.  
3. private void Start()
4. {
5.     actionQueue = new Queue<Func<bool>>();
6.     actionQueue.Enqueue(Encounter1);
7.     actionQueue.Enqueue(Encounter2);
8.     actionQueue.Enqueue(Encounter3);
9.     actionQueue.Enqueue(EncounterBanner);
10.     actionQueue.Enqueue(EncounterMiniboss);
11.     actionQueue.Enqueue(Encounter4);
12.     actionQueue.Enqueue(Encounter5);
13.     actionQueue.Enqueue(Encounter6);
14.     actionQueue.Enqueue(Victory);
15. }
16.  

Note that I'm storing a Func<bool> in the list. This means that your Encounter methods should return a bool. Why would they do that? So you can place the conditions in the method itself.

For example:

Code (csharp):

1. private float elapsedTime;
2. private float nextSpawnTime;
3. private List<IEnemy> activeEnemies;
4.  
5. private void Update()
6. {
7.     elapsedTime += Time.deltaTime;
8.  
9.     var nextAction = actionQueue.Peek();
10.  
11.     if (nextAction())
12.     {
13.         actionQueue.Dequeue();
14.  
15.         if (actionQueue.Count == 0)
16.         {
17.             this.gameObject.SetActive(false);
18.         }
19.     }
20. }
21.  
22. private bool EncounterBanner()
23. {
24.     if (activeEnemies.Count == 0)
25.     {
26.         SpawnBanner(Vector3.zero);
27.         nextSpawnTime = elapsedTime + 5f;
28.  
29.         return true;
30.     }
31.  
32.     return false;
33. }
34.  
35. private bool EncounterMiniboss()
36. {
37.     if (elapsedTime >= nextSpawnTime)
38.     {
39.         SpawnMiniboss(Vector3.zero, new Vector2(3f, 2f));
40.  
41.         return true;
42.     }
43.  
44.     return false;
45. }
46.  

You'll notice a few things in the example above:

• The update loop doesn't actually do anything. It just steps through the list of actions until it's empty.
• There is an activeEnemies list. When you spawn a new enemy, you add it to the list.
• The previous action sets the spawn time for the next action. As long as all of your encounters are called in a specific order, this will work okay. You'd want to look into more generic solutions so that you can easily reorder your encounters.

Why are we storing a list of IEnemy? So that any Enemy that inherits from IEnemy is guaranteed to have an OnDeath callback as shown below. We also use this to create a generic spawning method so as to reduce the number of places we manage the activeEnemies list:

Code (csharp):

1. private T Spawn<T>(T prefab, Vector3 position) where T : MonoBehaviour, IEnemy
2. {
3.     T enemy = Instantiate(prefab, position, Quaternion.identity) as T;
4.     enemy.OnDeath += EnemyKilled;
5.  
6.     EnemySpawned(enemy);
7.  
8.     return enemy;
9. }
10.  
11. private void EnemySpawned(IEnemy enemy)
12. {
13.     activeEnemies.Add(enemy);
14. }
15.  
16. private void EnemyKilled(IEnemy enemy)
17. {
18.     activeEnemies.Remove(enemy);
19. }
20.  
21. //New file
22. public interface IEnemy
23. {
24.      Action<IEnemy> OnDeath;
25. }
26.  

When you spawn the enemy, you also have the spawner subscribe to the enemy's OnDeath action so that the enemy can be removed from the 'activeEnemies' list when he dies. Because we're keeping track of spawns and deaths via the list, we no longer have to constantly check the game hierarchy for Enemies.

If you need to do something special to your spawned enemies, you can modify the public properties just as before since your base Spawn method returns the type. It's also good to note that you should reference your prefabs by their type if possible, as well as keep fields private and expose them only by '[SerializeField]'.

Code (csharp):

1. [SerializeField]
2. private StarryThing starryThingPrefab;
3. [SerializeField]
4. private MagnetThing magnetThingPrefab;
5.  
6. private MagnetThing SpawnMagnet(Vector3 position, Vector3 target)
7. {
8.     MagnetThing magnet = Spawn<MagnetThing>(magnetThingPrefab, position);
9.     magnet.TargetPosition = target;
10.  
11.     return magnet;
12. }
13.  
14. private StarryThing SpawnStar(Vector3 position, Vector3 target)
15. {
16.     StarryThing star = Spawn<StarryThing>(starryThingPrefab, position);
17.     star.TargetPosition = target;
18.  
19.     return star;
20. }
21.  

Summary:

1. Don't fill your Update loop with a bunch of conditions and returns. This becomes difficult to read and change.
2. Don't use GameObject.FindX if there's another way to get the information you're after. It's a relatively slow function, especially with large hierarchies.
3. Consolidate your methods. If you write the same code twice, there's probably a way to extract a method.
   
4. Use interfaces and abstract classes so that you can use generics when applicable. Don't go crazy.
5. Keep your fields private. Expose them to the inspector via "[SerializeField]". If you need access to a private field, create a property that exposes that field.
6. Don't call GetComponent for the same component in the same method. Cache the first call and use that.
7. Things I probably forgot.

Full source example: https://gist.github.com/Limyc-/6b0eb545957e994f969f

 

Nice code and good post.

 

@Taschenschieber I'm mostly going to parrot superpig. The two biggest improvements I would tackle are making the waves reusable and how the waves are managed. Figuring out how waves are managed is going to be an architectural issue that I would be having to choose between being poll driven or being event driven. Admittedly at the moment poll driven seems like the easiest solution as constantly checking to see if conditions have been met means it's easier to change conditions, but I'm not the one who needs this, so take some time to plot out what you need and what would be the best for this.

 

How convenient…

Good points all - there's some great discussion happening here. Throwing in my two cents on writing readable code:

1. Read code from other programmers - when you work only with your own code, you generally know what you want to happen so stuff makes sense. When you're coming at someone else's code with no prior mental context, you really start to understand the need for readable code.

2. Don't confuse comments with readability.
Often, programmers new to commenting tend to go crazy with it and religiously comment every single line, readability be damned.

Consider a simple enemy finding function:

Code (csharp):

1.  
2. GameObject[] EnemiesInRange(float r)
3. {
4.      GameObject[] es = GameObject.FindGameObjectsWithTag("Enemy");   //Find all the enemies
5.      List<GameObject> ea = new List<GameObject>();   //make a list to store the enemies in range
6.      foreach(var cur in es)   //loop through all the enemies
7.      {
8.           if(Vector3.Distance(cur, transform.position) < r) ea.Add(cur);  //add one if it's close enough to us
9.      }
10.      return ea;  //return the list of enemies
11. }
12.  

Code (csharp):

1.  
2. const string EnemyTag = "Enemy";
3.  
4. GameObject[] FindTaggedEnemiesWithinRadius(float radius)
5. {
6.      var allEnemies = GameObject.FindGameObjectsWithTag(EnemyTag);
7.      var enemiesInRange = new List<GameObject>();
8.  
9.      foreach(var currentEnemy in allEnemies)
10.            if(Vector3.Distance(currentEnemy.transform.position, transform.position) < radius)
11.                 enemiesInRange.Add(currentEnemy);
12.  
13.      return enemiesInRange.ToArray();
14. }
15.  

1. We use a const to store the enemy tag - this will save us from all typo errors down the line, and insure us against someone trying to change the enemy tag (just change the const value instead of going through every single instance).
2. We make it clear that this is only finding TAGGED enemies - not performing a component search or anything of the sort. We also make it clear that the single parameter is the radius we'll be searching in. Finally, we imply that this function will be performing work ("Finding") every time it is called.
3. var allEnemies… the case could be made for explicit typing here, since it could be non-obvious to a newcomer that GameObject.FindGameObjectsWithTag returns a GameObject[].
4. It's obvious from the assignment that enemiesInRange is a List of GameObjects, so there's no need to clutter the code with an explicit type.

**note: the above code is purely a demonstration and not suitable for actual use. Array allocation, non-cached components, magnitude rather than sqrMagnitude, etc.

 

Getting things back on track… Recently saw a challenge posted and couldn't help myself.

This snippet takes an array of IPs/hostnames and cycles through, pinging them all, then sorts by ping time and logs the result. A simple task made a bit more complex by Unity not properly supporting System.Net and Unity's Ping class not accepting hostnames out of the box.

Code (csharp):

1.  
2. using UnityEngine;
3. using System.Collections;
4. using System.Collections.Generic;
5. using System.Linq;
6. using System;
7.  
8. /// <summary>
9. /// Pings a list of servers and logs out in order of ping roundtrip time.
10. /// </summary>
11. public class PingTest : MonoBehaviour
12. {
13.     class PingData
14.     {
15.         public string originalURL;
16.         public string pingedURL;
17.         public long pingTime;
18.     }
19.  
20.     public List<string> urls = new List<string> () {
21.         "troggy.com",
22.         "apple.com",
23.         "localhost",
24.         "192.168.1.1"
25.     };
26.  
27.     void Start ()
28.     {
29.         StartCoroutine (PingServers (urls));
30.     }
31.  
32.     /// <summary>
33.     /// Loops through a list of ip addresses/hostnames and pings them. Coroutine is terminated when the last server has been pinged.
34.     /// </summary>
35.     /// <returns>The servers.</returns>
36.     /// <param name="urls">Urls.</
37.     IEnumerator PingServers (List<string> urls)
38.     {
39.         var pingResults = new List<PingData> ();
40.  
41.         //cycle through and ping all the servers
42.         foreach (var url in urls)
43.             StartCoroutine (PingURL (url, pingData => pingResults.Add (pingData)));    //send off the ping, and add the result to the array of results
44.                
45.  
46.         while (pingResults.Count < urls.Count)
47.             yield return null;        //wait for all pings to complete
48.                
49.         var logs = pingResults.OrderBy (pingData => pingData.pingTime)
50.                                         .Select (ping =>
51.                                         string.Format ("Pinged {0} for original url {1} with time: {2}", ping.pingedURL,
52.                         ping.originalURL,
53.                         ping.pingTime));
54.  
55.         Debug.Log (string.Join ("\n", logs.ToArray ()));
56.     }
57.        
58.  
59.     //TODO: add a timeout
60.     //TODO: add an exception for invalid urls
61.     //TODO: add error data for failed pings
62.     IEnumerator PingURL (string url, Action<PingData> completionHandler)
63.     {
64.         var urlHostName = url;
65.         if (!Uri.IsWellFormedUriString (urlHostName, UriKind.Absolute))
66.             urlHostName = "http://" + urlHostName;
67.         urlHostName = new Uri (urlHostName, UriKind.Absolute).Host;
68.  
69.         var ipAddy = System.Net.Dns.GetHostEntry (urlHostName).AddressList.First ();
70.         var ping = new Ping (ipAddy.ToString ());
71.         while (!ping.isDone)
72.             yield return null;
73.  
74.         if (completionHandler != null)
75.             completionHandler (new PingData { originalURL = url, pingedURL = ipAddy.ToString (), pingTime = ping.time });
76.     }
77.  
78. }  
79.  

 

"Static variables" and "statically typed variables" may sound similar, but it's a distinction you should really be aware of.

 

I don't like either of your code snippets tbh.

I'd prefer something similar to:

Code (csharp):

1.  
2. enum EntityType
3. {
4.    ENEMY_TYPE,
5.    FRIENDLY_TYPE,
6.    AMMO_TYPE,
7.    TRIGGER_TYPE,
8. }
9.  
10. IList<GameObject> GetEntitiesInRange(GameObject entity, float radius, enum EntityType entityType)
11. {
12.    IList<GameObject> validatedRange = new List<GameObject>();
13.  
14.    if (entity == null) return validatedRange;
15.  
16.    List<GameObject> inRange = octTree.RadiusQuery(entity.transform.position, radius);
17.    validatedRange = GetEntitiesOfType(inRange, ENEMY_TYPE);
18.  
19.    return validatedRange;
20. }
21.  
22. IList<GameObject> GetEntitiesOfType(IList<GameObject> entities, enum EntityType entityType)
23. {
24.    IList<GameObject> entitiesOfType = new List<GameObject>();
25.  
26.    foreach(GameObject entity in entities)
27.    {
28.       EntityType entityType = (EntityType)GameObject.GetProperty(EntityType);
29.       if (entityType != null)
30.       {
31.          entitiesOfType.Add(entity);
32.       }
33.    }
34.  
35.    return entitiesOfType;
36. }
37.