3
\$\begingroup\$

I am wondering if my code is needlessly redundant. I noticed, when looking over my code, that I had a unnecessary variable and if statement. Both Fire() methods achieve the desired result without error (from what I have seen). To me, the redundant Fire() method reads a lot nicer and is my preferable method but I'm just wondering if the alternate Fire() method is still worth the trade-off in readability due to efficiency. I am also wondering if there may be any underlying bugs that may occur in either of my methods that I am unaware of.

Unity version: 4.6.3

Fire()` method result: when the space key is pressed, it spawns a laser that fires upwards towards the enemy at the preset speed and is delayed by the reload time.

Desired result: when the space key is pressed, spawns a laser and has it fire in an upwards direction towards the enemies at the preset speed and than have a delay equal to the preset reload time.

Variables:

public float reloadTime = 0.2f;
public GameObject playerProjectilePrefab;
public float playerProjectileSpeed = 10f;

private bool canShoot = true;
private float reloadCountDown;

Redundant Fire Method:

void Fire (){
    Vector3 startPosition = transform.position + new Vector3(0 ,(renderer.bounds.size.y * 0.5f), 0);

    if (canShoot && Input.GetKey(KeyCode.Space)){
        canShoot = false;
        GameObject playerProjectile = Instantiate(playerProjectilePrefab, startPosition, Quaternion.identity) as GameObject;
        playerProjectile.rigidbody2D.velocity = new Vector3 (0, playerProjectileSpeed, 0);
        reloadCountDown = reloadTime;
    } else if (!canShoot){
        reloadCountDown -= Time.deltaTime;
    }
    if (reloadCountDown <= 0f){
        canShoot = true;
    }
}

Alternate Fire Method:

void Fire (){
    Vector3 startPosition = transform.position + new Vector3(0 ,(renderer.bounds.size.y * 0.5f), 0);

    if (reloadCountDown <= 0f && Input.GetKey(KeyCode.Space)){
        GameObject playerProjectile = Instantiate(playerProjectilePrefab, startPosition, Quaternion.identity) as GameObject;
        playerProjectile.rigidbody2D.velocity = new Vector3 (0, playerProjectileSpeed, 0);
        reloadCountDown = reloadTime;
    } else if (reloadCountDown > 0f){
        reloadCountDown -= Time.deltaTime;
    }
}
\$\endgroup\$
2
\$\begingroup\$

I would use the Alternate Fire Method but I would convert the canShoot field to a CanShoot property. Also startPosition is only used within the then block, so I would move its definition there as well. Example:

public bool CanShoot => reloadCountDown <= 0F;

private void Fire ()
{
    if (!Input.GetKey(KeyCode.Space))
    {
        return;
    }

    if (CanShoot)
    {
        Vector3 startPosition = transform.position + new Vector3(0 ,(renderer.bounds.size.y * 0.5f), 0);
        GameObject playerProjectile = Instantiate(playerProjectilePrefab, startPosition, Quaternion.identity) as GameObject;
        playerProjectile.rigidbody2D.velocity = new Vector3 (0, playerProjectileSpeed, 0);
        reloadCountDown = reloadTime;
    } 
    else 
    {
        reloadCountDown -= Time.deltaTime;
    }
}

Your code looks more Java-ish than C#. Since you've tagged it with C#, I will focus my comments on that. However, I am not familiar with Unity3d and don't know how it will like some syntax changes.

Braces on a new line

With C#, we prefer braces on their own line. The one exception would be a one-liner such as:

if (!Input.GetKey(KeyCode.Space)) { return; }

Fields should never be public

Your first 3 class-level variables are called fields. They should either be properties or constants. For example, will reloadTime always be 0.2f? If so, it can be a constant. If not, and if you want it to be exposed publicly, then you should make it a property. If reloadTime does not need to be public, then it can remain a field.

Method, Property, & Constant Names are CamelCase

Fields & Parameter Names are PascalCase

This doesn't YET apply to your original code, but let's combine it with the previous section. Your declarations could now look like:

public const float ReloadTime = 0.2F;
public GameObject PlayerProjectilePrefab { get; private set; }
public const float PlayerProjectileSpeed = 10F;

private bool CanShoot => reloadCountDown <= 0.F;
private float reloadCountDown;

But I am just guessing as to the private setters. Use what your app requires.

Numeric Type Designators should be Uppercase

Change the f to F. Really doesn't affect your code here but the reason for this guideline is you ever use long where a lowercase l could be confused as a numeric 1, but a capital L would not.

Always declare accessibility modifier

Instead of simply void Fire() use private void Fire(). When I see a private missing, I assume a newbie coder has forgotten it, which leads me to wonder what else he or she has forgotten.

Variable declaration and scoping

As mentioned at the very top with startPosition, you should limit the scope of a variable to where you need it. In my example, startPosition was placed inside the then block and declared immediately before the one place where it was referenced. Someone reading your code doesn't have to remember where it was defined, or worry about it afterwards.

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ In Unity, in the editor, you can attach a script to a GameObject. You can then adjust the values of the public fields for that GameObject, but not its properties, so they are not an option. An alternative is to have a private field with a SerializeField attribute above it. You could then add a property to access the private field for other classes if you wanted. \$\endgroup\$ – Risky Martin May 29 '16 at 14:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.