C# Creating a More Efficient Script

Questions & Answers
1

This has been driving me nuts. So I want to create a script that makes a gameobject rotate. I was able to achieve this but I still think this could be done better. I initially tried creating a vector2, assigning values to it and making the axes I don’t want to move 0. But with Mathf.PingPong it gave me an error saying the transform.position attempt was not valid. Do you guys have any suggestions for improvements of my script?

using UnityEngine;
using System.Collections;

public class Ping_Pong_Sway : MonoBehaviour {
	public float sway;
	public string swayAlongVector;
    void Update() {
		if(swayAlongVector == "x"){
		transform.localPosition = new Vector3(Mathf.PingPong(Time.time * 5, sway), transform.localPosition.y, transform.localPosition.z);
		}
		else if(swayAlongVector == "y"){
		transform.localPosition = new Vector3(transform.localPosition.x, Mathf.PingPong(Time.time * 5, sway), transform.localPosition.z);
		}
		else if(swayAlongVector == "x+y"){
		transform.localPosition = new Vector3(transform.localPosition.x, Mathf.PingPong(Time.time * 5, sway), transform.localPosition.z);
		}
    }
}
2

If your just trying to make it rotate in place (not around anything) you can just do something like this

tf.Rotate(-Vector2.up * rotateSpeed * Time.deltaTime);
3

You could use a switch statement

4

You just want to refactor your code?

First, I think you have a bug, you do the same for “y” and for “x+y”.

Second, I guess you could avoid most of that repetition writing it like this:

 using UnityEngine;
 using System.Collections;
 
 public class Ping_Pong_Sway : MonoBehaviour {
     public float sway;
     public string swayAlongVector;
     void Update() {
         //this check is not neccesary if swayAlongVector will alywas have one of this values
         if (swayAlongVector == "x" || swayAlongVector == "y" || swayAlongVector == "x+y") {
             float aux = Mathf.PingPong(Time.time * 5, sway); //write this only once

             Vector3 pos = Vector3.zero;
             pos.z = transform.localPosition.z;
         
             if(swayAlongVector == "x" || swayAlongVector == "x+y"){
                 pos.x = aux;
             }
             if(swayAlongVector == "y" || swayAlongVector == "x+y"){
                 pos.y = aux; 
             }

             transform.localPosition = pos;
         }
     }
 }

That code should work like your code.

Another improvement would be to use an enum instead of strings for the swayAlongVector variable. And would be even better if the enum is a “flags” enum: C# enum Flags Attribute Examples - Dot Net Perls

EDIT: Anyway, this code is not more “efficient”, that’s something you have to measure, but with something so simple it’s probably an insignificant difference. Your original code doesn’t create useless variables nor call the same function more than once, on each update only one of the conditions will be true, so only one line of code will get called. You should aim for cleannes, and maintainability instead of efficience in this case.

For example, it’s more maintainable if you write that PingPong line only once in case you have to change something (change in one place instead of 3). It’s easier to understand what the code does if you change the localPosition once at the end and use the swayAlongVector to only change the x and y values you’ll use. But it’s all subjective, someone might think other options are better for other reasons.

5

Make Sure that you didn’t set the sway value to be 0. Mathf.PingPong(float t, float length); the parameter require to be bigger than 0.