C# and unity. Moving an object to a target and destroy target once we're close

Question

This is my first ever c# script. I have played around a little and I think this is some proper c# code. Since it's my very first c# ever, I'm assuming there's a few mistakes, things that could be better, bad conventions and such. If you see any, please let me know so I can improve!

using System.Collections;
using System;
using System.Collections.Generic;
using UnityEngine;
using UnityEngine.AI;

public class Mover : MonoBehaviour
{
    [SerializeField] GameObject target;  
    private GameObject player; // creating a variable for the player object
    
    void Update()
    {
        if (target != null) { // if the target is already destroyed, we don't have to do anything anymore
            player = this.gameObject; // the script is attached to the player
            

            // Here I expected I could get the NavMeshAgent component via something like player.NavMashAgant. 
            // I do the same later for target.transform. 
            // Is there a reason this component is not accessible via dot notation?
            GetComponent<NavMeshAgent>().destination = target.transform.position; // set the players destination as the targets potion. It starts moving automatically. 
            if (AreClose(player, target)) { 
                Destroy(target);
            }
        }
    }

    private static double Distance(GameObject a, GameObject b)
    {
        Vector3 aPos = a.transform.position;
        Vector3 bPos = b.transform.position;
        // A really long line.. What is the convention to make this more readable?
        return Math.Sqrt(Math.Pow(aPos.x - bPos.x, 2) + Math.Pow(aPos.y - bPos.y, 2) + Math.Pow(aPos.z - bPos.z, 2));

    }

    private static bool AreClose(GameObject a, GameObject b)
    {
        double result = Distance(a, b);
        if (result < 2.5) // random magic number for now
        {
            return true;
        } else
        {
            return false;
        }
    }
}

Answer 1

Welcome to Code Review!

I'm not too familiar with Unity, so I can't really comment or offer advice on Unity-specific things.

There are still a few places where I feel like your code could be improved however.

Whitespace

While I generally prefer to keep my code not too dense, I recommend never having more than one blank line in a row (except maybe if you want to separate a block of imports or usings from the rest of the code); some editors will even automatically remove any consecutive blank lines when you format your document.

Similarly, that blank line at the end of your Distance() method doesn't serve any purpose, so I would just remove it, or maybe move it right below your aPos and bPos declarations.

AreClose() method

Your AreClose() method could be rewritten in a much shorter and cleaner way, like so :

private static bool AreClose(GameObject a, GameObject b)
{
    double result = Distance(a, b);
    return (result < 2.5); // random magic number for now
}

That's because the expression result < 2.5 itself evaluates to a boolean, so there's no need to check whether its value true or false, you can simply return it.

You could further shorten it to...

private static bool AreClose(GameObject a, GameObject b)
{
    return Distance(a, b) < 2.5; // random magic number for now
}

...since, in my opinion, the variable result doesn't add anything to the readability or the clarity of the code. Seeing Distance(a, b) < someMagicNumber is clear enough on its own.

You could go even further and take advantage of C#'s expression body definitions, like so

private static bool AreClose(GameObject a, GameObject b) => Distance(a, b) < 2.5; // random magic number for now

...which doesn't look too great here because of that comment. I would recommend you put that magic number into a const variable, either declared inside AreClose() or as a class variable. This will make your code easier to modify in the future, and also allows you to give it a meaningful name like MIN_DISTANCE.

private static bool AreClose(GameObject a, GameObject b) => Distance(a, b) < MIN_DISTANCE;

Distance() method

Like I said earlier, I'm not too familiar with Unity, but I'm pretty sure it has a built-in method for computing the distance between two vectors, like so Vector3.Distance(aPos, bPos).

There's nothing inherently wrong with doing it yourself if you're just trying to learn and get familiar with a new language or concept, but if you're looking for optimal performance or accuracy, I would recommend making sure there isn't already a function that does what you need before trying to reinvent the wheel -although in this specific case it's probably not too important.

Additionally, I would avoid using Math.Pow() when you only want to square a number, as it's quite an expensive operation compared to simply doing (aPos.x - bPos.x) * (aPos.x - bPos.x). This is because Math.Pow() needs to be able to handle non-integer exponents and (probably) uses Taylor series (https://en.wikipedia.org/wiki/Taylor_series) to compute powers for arbitrary exponents, even if your exponent happens to be 2. So you can just compute the square yourself, which should significantly improve performance.

You could also create a DistanceSquared() method, and compare it with MIN_DISTANCE * MIN_DISTANCE, which is equivalent to comparing Distance() and MIN_DISTANCE. This allows you to avoid using Math.Sqrt(), which is, again, a rather slow operation. Then you can rewrite Distance() like this, if you still want it :

public static double Distance(GameObject a, GameObject b) => Math.Sqrt(DistanceSquared(a, b));

Answer 2

Unity has built in support for measuring distance. If I remember correctly it would look something like this.

return (a.transform.position-b.transform.position).Length

You could also save on performance by squaring the radius to check instead of sqrt ing the distance.

Branches are uneccessary in this function.

private static bool AreClose(GameObject a, GameObject b)
{
    return Distance(a, b) < 2.5;
}

The unity inspector has really good support for editing primitive variables if you make them public.

public class Mover : MonoBehaviour
{
  public float destroyRadius = 2.5f;
...

GetComponent<NavMeshAgent>()

GetComponent is kind of slow. Assigning it in Start() would help performance.