patterncsharpMinor
Player projectile shooting
Viewed 0 times
playerprojectileshooting
Problem
I am wondering if my code is needlessly redundant. I noticed, when looking over my code, that I had a unnecessary variable and
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:
Redundant Fire Method:
Alternate Fire Method:
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){
reloadCountDown -= Time.deltaTime;
}
}Solution
I would use the Alternate Fire Method but I would convert the
Your code looks more Java-ish than C#. Since you've tagged it with
Braces on a new line
With C#, we prefer braces on their own line. The one exception would be a one-liner such as:
Fields should never be public
Your first 3 class-level variables are called
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:
But I am just guessing as to the private setters. Use what your app requires.
Numeric Type Designators should be Uppercase
Change the
Always declare accessibility modifier
Instead of simply
Variable declaration and scoping
As mentioned at the very top with
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.Code Snippets
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;
}
}if (!Input.GetKey(KeyCode.Space)) { return; }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;Context
StackExchange Code Review Q#129591, answer score: 2
Revisions (0)
No revisions yet.