Feedback What is the redundant part in my code?

I feel like there are some unnecessary parts in my code. Can you tell me where it is or can you tell me the bad parts in terms of performance? Thank you.

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4.  
5. public class Solider : MonoBehaviour
6. {
7.     private Animator anim;
8.     [SerializeField] private LayerMask layerMask;
9.     [Header("Movement")]
10.     private bool _running;
11.     [SerializeField] private float runSpeed;
12.     [SerializeField] private float rotateSpeed = 20f;
13.     private bool startMovement = false;
14.     private Transform target;
15.     private int currentWaypoint = 0;
16.     [Header("Attack")]
17.     [SerializeField] private float attackRange;
18.     public bool isEnemy = false;
19.     RaycastHit hit;
20.     //Timer
21.     [SerializeField] private float attackCooldown = 2;
22.     private float currentTime = 0;
23.  
24.  
25.  
26.  
27.     public void StartProgress()
28.     {
29.         anim = GetComponent<Animator>();
30.         Running = true;
31.         if (isEnemy)
32.         {
33.             currentWaypoint = Waypoints.waypoints.Length - 2;
34.  
35.             this.gameObject.tag = "Enemy";
36.         }
37.         else
38.         {
39.             currentWaypoint = 0;
40.             this.gameObject.tag = "Player";
41.         }
42.  
43.         target = Waypoints.waypoints[currentWaypoint];
44.         startMovement = true;
45.     }
46.     void Update()
47.     {
48.         if (startMovement)
49.         {
50.             Vector3 dir = target.position - transform.position;
51.             dir.y = 0;
52.             transform.Translate(dir.normalized * runSpeed * Time.deltaTime, Space.World);
53.             transform.rotation = Quaternion.RotateTowards(transform.rotation, Quaternion.LookRotation(dir), Time.deltaTime * rotateSpeed);
54.  
55.             if (Vector3.Distance(transform.position, target.position) < 1)
56.             {
57.                 UpdateWaypoint();
58.             }
59.         }
60.         if (Physics.Raycast(this.transform.position, this.transform.forward, out hit, 2, layerMask))
61.         {
62.             startMovement = false;
63.             Running = false;
64.             if (hit.transform.gameObject.tag == "Player" && isEnemy || hit.transform.gameObject.tag == "Enemy" && !isEnemy)
65.             {
66.                 currentTime -= Time.deltaTime;
67.                 if (currentTime <= 0)
68.                 {
69.                     Attack();
70.                     currentTime = attackCooldown;
71.                 }
72.             }
73.         }
74.         else
75.         {
76.             startMovement = true;
77.             Running = true;
78.         }
79.     }
80.  
81.  
82.     private void UpdateWaypoint()
83.     {
84.         if (isEnemy)
85.         {
86.             if (currentWaypoint <= 0)
87.             {
88.                 Destroy(gameObject);
89.                 return;
90.             }
91.             currentWaypoint--;
92.         }
93.         else
94.         {
95.             if (currentWaypoint >= Waypoints.waypoints.Length - 1)
96.             {
97.                 Destroy(gameObject);
98.                 return;
99.             }
100.             currentWaypoint++;
101.         }
102.         target = Waypoints.waypoints[currentWaypoint];
103.     }
104.     private void Attack()
105.     {
106.         anim.SetTrigger("Attack");
107.     }
108.     public bool Running
109.     {
110.         get { return _running; }
111.         set
112.         {
113.             if (value == _running) return;
114.             _running = value;
115.             anim.SetBool("Running", _running);
116.         }
117.     }
118. }
119.  

 

Why do you feel this way?

I'm looking for you to say... "I think this part from line A to B could be simpler because all it does is X."

If you aren't sure, that must be because YOU haven't taken the time to actually understand YOUR code.

In that case I'm curious why you think WE should take OUR time to try and understand your code?

Otherwise, how to report your problem productively in the Unity3D forums:

http://plbm.com/?p=220

This is the bare minimum of information to report:

- what you want
- what you tried
- what you expected to happen
- what actually happened, especially any errors you see
- links to documentation you used to cross-check your work (CRITICAL!!!)

You may edit your post above.

 

I understand the whole code and it has no problem in its working. I wrote most of it myself (not all).I just had the feeling that something was wrong but I couldn't detect it so I wanted to ask.

 

@Kurt-Dekker is right, having others read through your working code for no specific reason other than to say if it's good or bad, is not very nice, because it's basically asking for a chunk of our time, that should actually be your own time.

Anyway I went through your code and I gotta admit, it was a fun read:

Oh boy, where do I even start?
First of all, the naming convention of this code is just all over the place. Some variables have underscores, some don't. The use of "Solider" instead of "Soldier" just shows a lack of attention to detail. And personally - but that's just me - I would never use a word like "progress" in a method name.

Moving on to the actual code, there are quite a few areas that could be optimized for performance. For example, you're using GetComponent<Animator>() in the StartProgress() method, which is bad practice - cache the component in a private variable instead.

The use of fixed numbers (such as the "1" in the distance check) can make your code more prone to bugs sneaking in as soon as you add or edit code later on. Use named constants instead, or at least comment your code to explain what the numbers are.

Also, the use of raycasts can be very performance-heavy if overused. In this case, you're doing a raycast every frame in the Update() method, which could be a problem if you have a lot of soldiers in your scene. You should consider using colliders to detect collisions instead, that's what they're for.

I couldn't figure out what the purpose of the "isEnemy" variable is, to save my grandma.

The "startMovement" variable is being set to true in the "StartProgress" method but is never reset to false after the object reaches the last waypoint. Do you want it to move indefinitely? Isn't it redundant since you already have the "Running" property, which can be used to control the movement of the solider?

Speaking of which, the "Running" property is clearly the star of the show. Because why use the actual bool value when you can add an unnecessary property? Then again, you could use it to get rid of the startMovement variable, which would give it some purpose after all (or not? ooooh... my head...)

Looks like the "UpdateWaypoint" method just loves repeating itself. Duplicate code is the spice of life!

The "Attack" method is not actually attacking anything, it is just triggering an animation. IDK if this is the intended behavior or if there should be some actual attack happening.

"currentTime" is not getting initialized properly and being decremented instead of incremented. The "currentTime" variable should also be reset to "attackCooldown" after the attack animation has been triggered. Otherwise, the cooldown could be skipped entirely if the attack animation lasts longer than the cooldown period.

Now jokes aside, do not let this pull you down, you can do it all, but it's most important that you first spend some more time on your own, banging your head against the wall to figure those things out yourself and only after that get on the forum and ask questions more specific.

Happy coding!

 

Even if it is redundant....it's a couple lines...you're not going to be able to tell performance wise. If it isn't broke...don't fix it.

 

Questions like these without any context included tend to be difficult to do more than provide a basic analysis of the code and if you just want that I recommend passing it to ChatGPT. Here's what it suggested for your code.

Just profile before (and after) you decide to make any adjustments because it's very difficult to truly know at a glance if your code has performance problems. All of the below recommendations are very minor in my opinion.

Spoiler: ChatGPT

Overall, your code seems well-written and straightforward. However, there are some areas that can be improved in terms of performance:

1. Raycasting In the Update() method, you are performing a raycast on every frame, which can be expensive, especially if there are many objects in the scene. Consider reducing the frequency of raycasts or caching the result of the raycast and only updating it when necessary.
   
   
2. Quaternion.RotateTowards In the Update() method, you are using the Quaternion.RotateTowards method to smoothly rotate the character towards its target. This method can be computationally expensive, especially if there are many characters in the scene. Consider using simpler methods such as Vector3.Lerp to achieve the same effect.
   
   
3. GetComponent In the StartProgress() method, you are using the GetComponent method to retrieve the Animator component. This method can be expensive, especially if there are many objects in the scene. Consider caching the component in a variable in the class to reduce the frequency of calls to GetComponent.
   
   
4. Magic Numbers There are some magic numbers in your code, such as the "1" value used in the distance check and the "2" value used in the raycast. Consider replacing these values with named constants or variables to improve code readability and maintainability.
   
   
5. Code Duplication There is some duplicated code in the UpdateWaypoint method. Consider extracting the common code into a separate method to reduce redundancy.

Overall, your code seems well-written, and these suggestions are minor optimizations that can be applied to further improve performance.

 

Wow thank you for taking your time for reviewing start to finish.

I'm solo dev thats why generally i name stuff like i will understand in the future but thank you about that suggestion.

The StartProgress method is calling once on awake. I set isEnemy bool from another script and i need to set it in first frame before code start but in awake it was not being possible thats why i made that method so i can set enemy and then start code.

Code (CSharp):

1.         if (Input.GetKeyDown(KeyCode.D))
2.         {
3.             Solider soliderScript = Instantiate(solider, enemySpawnPos.position, enemySpawnPos.rotation).GetComponent<Solider>();
4.             soliderScript.isEnemy = true;
5.             soliderScript.StartProgress();
6.  
7.         }

like this.

About raycast before using that i tryed a lot more stuff like k-dTree, physics.overlapsphere, ontrigger but they didn't work very well in my game. Im making game like this

isEnemy like i told i set from another script because player soliders and enemy soliders has same script.

About startMovement yes its unnecessary Running doing the same thing so i delete it, thank you.

That Running method from internet copy paste. I didnt want to setbool every frame so one guy suggested something like that method.

And code is not fully finished yet thats why parts like attack is missing.

 

Thank you everyone for answers.