I would like to know if this script is well written.

This script that works allows you to create a delay between two Input.GetKeyDown, I would like to know your opinion, on the way it is written, does it seem consistent and optimized ?

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4.  
5. public class Bullet: MonoBehaviour
6. {
7.    
8.     public GameObject myBullet;
9.     private float cd0 = 0;
10.     [SerializeField] [Range (0, 5)] private float cooldown = 1f;
11.    
12.     void Update()
13.     {
14.        
15.         if (Input.GetKeyDown(KeyCode.Space) && cd0 <=0)
16.         {
17.             cd0 = cooldown;
18.             Instantiate(myBullet, transform.position, myBullet.transform.rotation);
19.    
20.         }
21.         if (cd0 >= 0)
22.             {
23.             cd0 -= Time.deltaTime;
24.             }
25.     }
26. }
27.  

 

Does it work? Ship it!

There's almost nothing above to reason about. Is it just a bog-standard cooldown timer?

Cooldown timers, gun bullet intervals, shot spacing, rate of fire:

https://forum.unity.com/threads/fire-rate-issues.1026154/#post-6646297

GunHeat (gunheat) spawning shooting rate of fire:

https://forum.unity.com/threads/spawning-after-amount-of-time-without-spamming.1039618/#post-6729841

Otherwise, DO NOT OPTIMIZE "JUST BECAUSE..." If you don't have a problem, DO NOT OPTIMIZE!

If you DO have a problem, there is only ONE way to find out. Always start by using the profiler:

Window -> Analysis -> Profiler

Failure to use the profiler first means you're just guessing, making a mess of your code for no good reason.

Not only that but performance on platform A will likely be completely different than platform B. Test on the platform(s) that you care about, and test to the extent that it is worth your effort, and no more.

https://forum.unity.com/threads/is-...ng-square-roots-in-2021.1111063/#post-7148770

Remember that optimized code is ALWAYS harder to work with and more brittle, making subsequent feature development difficult or impossible, or incurring massive technical debt on future development.

Notes on optimizing UnityEngine.UI setups:

https://forum.unity.com/threads/how...form-data-into-an-array.1134520/#post-7289413

At a minimum you want to clearly understand what performance issues you are having:

- running too slowly?
- loading too slowly?
- using too much runtime memory?
- final bundle too large?
- too much network traffic?
- something else?

If you are unable to engage the profiler, then your next solution is gross guessing changes, such as "reimport all textures as 32x32 tiny textures" or "replace some complex 3D objects with cubes/capsules" to try and figure out what is bogging you down.

Each experiment you do may give you intel about what is causing the performance issue that you identified. More importantly let you eliminate candidates for optimization. For instance if you swap out your biggest textures with 32x32 stamps and you STILL have a problem, you may be able to eliminate textures as an issue and move onto something else.

This sort of speculative optimization assumes you're properly using source control so it takes one click to revert to the way your project was before if there is no improvement, while carefully making notes about what you have tried and more importantly what results it has had.

 

You might want to do

cd0 += cooldown;

instead of setting it to cooldown, so the "rate" is correct (now it's going to be slightly slower depending on framerate).

 

https://docs.microsoft.com/en-us/do...ls/coding-style/coding-conventions#camel-case

"Use camel casing ("camelCasing") when naming private or internal fields, and prefix them with _"

Code (CSharp):

1. private float cd0 = 0;

should be

Code (CSharp):

1. private float _cd0 = 0;

You don't need the prefix for a private serialized field though.

Also, cd0 is a bad name. It should be more explicit in stating what it does (cooldown?)

I'm also sure that conventions dictate private comes before public. so

Code (CSharp):

1. private int _number;
2. public string letter;

not

Code (CSharp):

1. public string letter;
2. private int _number;

Does anything inherit the bullet class? if not, seal it.

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/sealed

I believe you get a tiny performance because it doesn't have to perform certain checks.

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4.  
5. public sealed class Bullet: MonoBehaviour
6. {
7.     private float _cd0 = 0;
8.  
9.     [SerializeField] [Range (0, 5)] private float cooldown = 1f;
10.  
11.     public GameObject myBullet;
12.  
13.     private void Update()
14.     {
15.  
16.         if (Input.GetKeyDown(KeyCode.Space) && cd0 <=0)
17.         {
18.             _cd0 = cooldown;
19.             Instantiate(myBullet, transform.position, myBullet.transform.rotation);
20.  
21.         }
22.  
23.         if (_cd0 >= 0)
24.         {
25.             _cd0 -= Time.deltaTime;
26.         }
27.  
28.     }
29. }

 

Thank you for your reply, and I apologize if I didn't use the right words.

I learn and understand quickly, but I'm still a beginner.

The script works, in the sense that I wanted a script that would create a delay between my inputs, unfortunately I don't have your level of knowledge, to hope to optimize at this level.

I just wanted to know if the code doesn't look badly written to you, unfortunately I don't have enough perspective and knowledge to adopt the right programming methods, I'm improving every day ^^ and I wanted to have an outside opinion.

But I realize the work I still have to do.


AcidArrow I didn't quite understand but I'll find out ^^ Thank's.


Thank you Jnsptz, it's great, I have a good working base to improve myself ^^

 

You don't have to apologise. I was just pointing out ways you could improve (which is what I thought you were asking for). You can see a list of conventions here:

https://docs.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/coding-conventions

Doesn't really matter what I think to be honest. The only thing that matters is does it work. That might be different if you work in a team but everyone writes imperfect code really. Unless you're working on something professional or planning to ship a game I wouldn't worry about it.

 

You immediately understood my request, adopting good habits is not a bad thing ^^ but I remembered what you told me: camelCasing on private variables, not useful on serialized ones, private ones first , not well understood the role of the sealed, but I will find out ^^ I will pay more attention to the coding convention Thank's.

 

You think cd0 was not a good choice?
Since I already have a cooldown variable, I didn't want to confuse it, but yes it is cooldown0.


If I separate my variables with

[Header( "My text" )]

, I guess the convention would be I write my privates first, then in my header I enter the [SerializeField]private
then the public?

 

Not really but it can be subjective. If it's just you then might not matter - although you might pickup bad habits that will only be noticeable in the future if you work on larger projects or other people.

Code (CSharp):

1. _cd0 = cooldown;

It's still confusing to be honest. If I understand what you're using it for, it might be easier to distinguish like this.

Code (CSharp):

1. _
2. private float _waitTime = 0;
3.  
4. _waitTime = cooldown;

In any case, it's up to you. Getting it to work is the only important thing.

I think it would be like this, with private split from serialized private because they don't show in the inspector.

Code (CSharp):

1.  
2. private int _number;
3.  
4. [Header("Example")]
5. [SerializeField] private string placeholder;
6.  
7. [Header("Example 2")]
8. public string example;
9.  

Whether it's worth rigidly following convention is up to you. You could create your own convention if you want - most people come to have their own style of coding to some extent.

 

Rigidly following convention is bad IMO, because then you come across a team that uses a different style and you’re having a hard time to adapt.

Which is the important thing here: being able to adopt a certain style when working with other people is important. If you’re on your own, just do whatever makes sense to you.

 

In my opinion none of that matters. The main reasons have largely been touched on. Consistency requires looking at more than just one script, and optimization requires profiling before trying to fix anything as even an experienced developer will rarely know the true cause of the bottleneck let alone whether their changes improved anything.

What truly matters is whether the script works allowing you to ship your game, and if you are able to understand it months after you've written it. The last thing you want to do is put a ton of effort into a script only to have no idea how it works a few months later when you need to make changes to it.

It's not just because you might have to rewrite your script and code reliant on it but also that you might introduce bugs in the process of trying to work with it thanks to not having a complete understanding of why you wrote it the way you did.

 

My suggestion is that _cd0 is not a descriptive variable name. It's better to name a variable something that indicates what it is used for.

 

Code (CSharp):

1.  
2. // removed unused usings
3. using UnityEngine;
4. // namespaces help you not break any code in other assemblys when
5. // using the same ClassName. If you have a class called "Event"
6. // it would override the "Event"-class from UnityEditor for example
7. // you can prevent this by adding a namespace. You should always add
8. // a namespace to all classes you write.
9. // ASDF = usually the first letters of your company or name
10. namespace ASDF
11. {
12.     // why is it named "Bullet" if it takes inputs from the player?
13.     // I don't see any collision or raycast logic here, which is what
14.     // I would expect from a bullet
15.     // this seems more like a weapon than a bullet
16.     public class Bullet : MonoBehaviour
17.     {
18.         // why is this called "MY Bullet"?
19.         // it's just the bullet that get's fired form .. this bullet? or weapon?
20.         // so I'd just call it "bullet"
21.         public GameObject myBullet;
22.         // renamed from "cd0" to "cooldownTimer"
23.         // and removed "private" because that's the default
24.         // so you don't have to explicitly specify it.
25.         // less code usually looks cleaner
26.         float _cooldownTimer = 0;
27.         [SerializeField][Range(0, 5)]
28.         float _cooldown = 1f;
29.         void Update()
30.         {
31.             // KeyCode.Space is hardcoded here.
32.             // If you ever want your player to be able to change the keybindings
33.             // you will have to rewrite this class.
34.             // You should use the "new InputSystem" from Unity instead
35.             // or if you really want to go with the old system, write and use a wrapper.
36.             if (Input.GetKeyDown(KeyCode.Space) && _cooldownTimer <= 0)
37.             {
38.                 _cooldownTimer = _cooldown;
39.                 // Instantiate - how many bullets will you spawn?
40.                 // this seems like a single shot weapon so it might be OK here
41.                 // but If the player can have more than one of those weapons
42.                 // I'd suggest using a pool instead. Unity has released a
43.                 // robust pooling system a while ago.
44.                 Instantiate(myBullet, transform.position,
45.                     // myBullet.transform.rotation?
46.                     // you are reading the rotation of the bullet prefab here.
47.                     // And you are spawning it at the same position as the weapon?
48.                     // Shouldn't there be a Transform muzzlePoint; of some sort,
49.                     // where you get the position and rotation?
50.                     // if the rotation doesn't matter, I'd go with Quaternion.identity
51.                     myBullet.transform.rotation);
52.             }
53.             if (_cooldownTimer >= 0)
54.             {
55.                 _cooldownTimer -= Time.deltaTime;
56.             }
57.         }
58.     }
59. }

 

Since most have already given some style advices, I want to drop my two cents

First of all it's great to have short, simple and concise scripts. As others have already mentioned you should be a bit more careful with names. Classnames should be descriptive so it's clear what they do or represent. It's oftent difficult to come up with good names, especially for behaviour scripts that do not represent certain kind of objects but behaviour like this one. The name "Bullet" would natually be associated with a script on a bullet object. Most similar scripts I've seen had names like "ShootingScript" or something like that since the script is just about the shooting.

I prefer having the "Prefab" suffix for variables that hold prefab references. It makes it easier to distinguish between "normal" cross references between objects. So it's clear when wiring up the class in the inspector what goes into the variable and for what purpose.

Attributes can be in the same line as the variable declaration, however it's usually much better for the readability to place them above the declaration. That keeps the access modifiers and the variable types roughly at the same indention. Also when having multiple (short) attributes you can put them in the same line, though you don't need seperate square brackets for them, you can simply seperate them with commas. So I would do either

Code (CSharp):

1. [SerializeField, Range (0, 5)]
2. private float cooldown = 1f;

or

Code (CSharp):

1. [SerializeField]
2. [Range (0, 5)]
3. private float cooldown = 1f;

Performance wise there is not much to say with such a simple script. However there are two micro optimisations you can do. First when having multiple conditions in a single if statement, it's better to order them from simplest to more complex so you get an early exit. So the cooldown check may be placed first before the GetKeyDown check. The other second thing is that the second if statement it kinda redundant from a logical point of view. By reversing the order you can simply use an else if statement like this:

Code (CSharp):

1. public class ShootingScript : MonoBehaviour
2. {
3.  
4.     public GameObject bulletPrefab;
5.     private float timer = 0;
6.     [SerializeField]
7.     [Range (0, 5)]
8.     private float cooldown = 1f;
9.  
10.     void Update()
11.     {
12.         if (timer > 0)
13.         {
14.             timer -= Time.deltaTime;
15.         }
16.         else if (Input.GetKeyDown(KeyCode.Space))
17.         {
18.             timer += cooldown;
19.             Instantiate(bulletPrefab, transform.position, transform.rotation);
20.         }
21.     }
22. }
23.  

So here the condition

timer <= 0

is implicitly checked through the "else".

Finally something that we can not really judge on but it just seems strange: Why do you use the prefabs original orientation when you instantiate the bullet? Shouldn't the orientation be based on the orientation of this shooting gameobject? That way the bullet would be rotated based on how this object is placed in the scene. So I replaced

myBullet.transform.rotation

with just

transform.rotation

.

Those are just some minor points and some are just my own twist on this topic. So don't take them as general rules but just as an advice.

 

Jnsptz,

_waitTime

Good idea, I hadn't thought of that.


The camelCasing seems to me to be good for immediately identifying what type of variable has to do, as well as putting the private variables first, on the other hand for the [Header], since I use it as a separator, I think that this convention is more ergonomic =

• Header section 1: SerializeField private / public
• Header section 2: SerializeField private / public
• ....

AcidArrow
This type of convention is not very complicated to implement, but I understand what you wanted to tell me.

The formatting allows better readability and I'm not going to deprive myself of it, but I still learned a few little tricks like the sealed ... which may not have much importance on a small script like this, but a seems a little more important in big projects.



Ryiah, you were right, for the moment I am taking the time to learn little by little ^^, I am aware of the importance of optimizing your games, but even if this may be a mistake, I put that aside for now, I have a lot of information to learn and understand before reaching this level.
I am not native English, and I used the word optimization in a generic sense, to ask if the code does not present any gross error, I am aware of not having used the right words, but the result is that I learned a lot of things, but I'm happy ^^


Didn't think I blindly follow convention. I have a logical mind, so far, ormis in the headers, nothing that I have been told seems absurd to me, quite the contrary. ^^
I don't adopt a thought pattern without understanding it, fortunately


For the profiler, I heard you, but I'm not there yet, for now I have a good command of unity tools, I'm trying to improve my understanding as well as the writing of the code, for create the necessary tools for my future needs ^^


kdgalla I don't think I can find anything better than:

_waitTime

from Jnsptz. ^^


Hannibal_Leo

Thank you for your comments which I find very interesting. I will take the time to study them ^^ Do not formalize yourself too much on the name of the script, originally it is called: InstantiateWithKeypad.cs and my game object: projectilePrefab, I changed the names for this test project, it may be a mistake ^^ I don't use raycast on it, I use this script to generate physical projectiles, don't forget that my level is low, so I start by making it simple ^^ For the moment I prefer to create several small simple and ergonomic scripts, I try to think about the future need I might have to add inspector variables. It's easier for me, for now, to create a new big script from my many little scripts I've created. Or to test with them. I read all your comments, which I find extremely interesting, but I repeat myself, do not formalize yourself on the name Bullet, for me this script must create an object every X seconds, the rest like velocity for example is created with another script.


Bunny83
You are doing well ^^ I will not complain ^^

As explained above, I renamed this script for the needs of a project, but I agree bullet is not necessarily the most suitable. Yes me too normally I add the suffix Prefab, but I forgot ^^ like what, I can make beautiful speeches on conventions, I don't really respect them when I write quickly :/ I don't know this way of writing attributes, very interesting thank you. Actually not stupid, I don't think like that yet, but thank you.

timer <= 0

??

Well seen, for transform.rotation, it's just an error, as I did a copy paste (I know it's not good..), I didn't pay attention.

Don't worry, all the advice from all the speakers on this forum helps me progress, and I thank you all.

 

I adhere to https://docs.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/coding-conventions because most of the code I am going to touch from Unity, or newer code using C# will also adhere to it.

Since Microsoft are also responsible for the development of C# I see no reason to object.