Feedback for a code snippet

feedback.png

Feedback_EN.cs

// This is feedback and refactoring of a code snippet. You can see the original in feedback.png

public class Feedback_EN : MonoBehaviour
{
    // Whenever we define a variable it allocates some memory and deallocates it back once we are done with the
    // variable. This is called garbage collection, and a relatively slow process. Of course, a single ray won't 
    // affect the performance. In fact, it is called "micro-optimization." But why not  do it if we can
    private Ray _ray;

    // Similarly, we don't need to redefine Raycast hit in each frame
    private RaycastHit _hit;

    // Camera.main was a very expensive process. It was optimized recently, but I didn't check the
    // performance in the newer versions. Still, I don't think it will hurt to store it in a variable
    private Camera _cam;

    // Usually, when we perform a raycast we do it against a specific set of objects. In this case, it looks like
    // it should be something like "Ground." So the mask can be set only to the "Ground" layer, or just ignore 
    // some irrelevant layers. This is a private field because no one outside this script should care about it.
    // But since we want to set it in the Inspector window, we should expose it. [SerializeField] attribute does exactly that.
    [SerializeField] private LayerMask mask;
    
    // Raycast is a relatively expansive operation. So we want to optimize it as much as we can. Setting maximum
    // length is one of those optimizations. Unless we want to shoot a ray to infinity, we always should specify
    // its length.
    [SerializeField] private float rayLength = 10;

    // We want to check whether or not the agent is this close to its destination. The "Sqr" at the end indicates
    // that it is a square of the actual value. This is another optimization, and I will talk about it later
    [SerializeField] private float arriveDistanceSqr = 0.01f;

    // I assume that the agent was set somewhere in the original script.
    [SerializeField] private Agent agent;

    private void Awake()
    {
        _cam = Camera.main;
    }

    // In the original snippet, the variable names were in Turkish. It is a very bad idea to use any language except
    // English in the code. At some point, you might want to ask something online. People will have a very hard time
    // understanding your code. Or you may hire or collaborate with someone who doesn't speak your language. Then
    // you will have a problem and many hours of lost productivity. So I have translated names to English.
    private void Update()
    {
        // Checking for mouse input is much cheaper than raycasting. And obviously, we want to do stuff only when we
        // click somewhere. So checking for the mouse button first makes more sense.
        if(Input.GetMouseButtonDown(0))
        {
            _ray = _cam.ScreenPointToRay(Input.mousePosition);
            if(Physics.Raycast(_ray, out _hit, rayLength, mask))
            {
                // Checking or searching for game objects by name is a very very bad idea. String operations are
                // relatively expansive, we can make a mistake when writing it in code. And if the name of the object
                // changes, we will have to search through all of the code-base to find where we used it. But if you
                // really need to use a string at least do what I did here. For the full explanation check out my blog
                // over here: https://www.anton.website/quick-tip-for-constants/
                // Now you can't make a spelling mistake, and if you need to change it you will need to do it from a single place.
                if(_hit.transform.name == Constants.Strings.floor)
                {
                    agent.destination = _hit.point;
                }
            }
        }

        // In the original script agent.destination was compared to _hit.point. It doesn't make any sense, because we just
        // assigned agent.destination to _hit.point. So the values obviously will be the same. I assume the purpose was to
        // check whether or not the agent reached its destination. So we need to compare agent.destination with its current
        // position. We could use the == operator, but it is a bad idea. Float calculations are not super precise. Even if
        // you see 0 in the inspector, and debug log prints out the value of 0.0, it actually could be 0.0000001 or something.
        // Which is obviously not 0. So it is a good idea to use Mathf.Approximately(a, b) when we compare two floats. The
        // same thing applies to Vector3 since it is only a set of three floats. However, we won't be checking for the exact
        // values, because it is unlikely that all three floats will be exactly what we want them to be. Instead, we will
        // check the proximity or distance between two points. If it is smaller than a certain value, we can assume that the
        // agent has arrived at its destination.

        // We could have used Vector3.Distance, or Vector3.Magnitude over here (they mean the same thing). However,
        // calculating a distance between two points, or a length of a vector requires taking a square root at one point.
        // Taking a square root is an expansive operation. But when we use Vector3.DistanceSqr we skip the square root part
        // and get the squared distance as a result. Unless we want to know the exact distance we should always avoid
        // calculating the magnitude of a vector.
        if(Vector3.DistanceSqr(agent.destination, agent.transform.position) <= arriveDistanceSqr)
        {
            Debug.Log("Arrived!");
        }
    }
}

Feedback_TR.cs

// Bu geribildirimi gist'e ekli olan feedback.png dosyasindaki kod parcasina verdim

public class Feedback_TR : MonoBehaviour
{
    // Herhangi bir degiskeni tanimladigin zaman ram'de buna yer ayriliyor, isin bitince de o yer
    // ram'e geri veriliyor. Buna garbage collection deniyor, ve agir bir islem. Tabi tek bir ray
    // performansi etkilemeyecektir, ve bu bir "mikro optimizasyon" ama firsatin varsa neden yapmayasin
    private Ray _ray;

    // Benzer sekilde RaycastHit'i de her frame'de tanimlamamiza gerek yok
    private RaycastHit _hit;

    // Camera.main islemi son zamanlarda optimize edilmis olsa da hala biraz pahali olabilir. Son
    // surumlerdeki performans farkina bakmadim, ama bir kere atayip hep onu kullanmakta fayda var.
    private Camera _cam;

    // Raycast islemini yaparken genelde spesifik objelere karsi yapiyorsun. Senin durumunda karakterin
    // yuruyebilecegi alanlar bunlar. Cok buyuk ihtimalle o alanlar spesifik bir layer altinda toplanilabilir.
    // Bu degiskene private dememin bu script disinda kimsenin ilgilenmiyor olmasi. Ancak bu degiskeni
    // Inspector penceresinden atayabilmek istiyoruz, o yuzden basina [SerializeField] yazdim.
    [SerializeField] private LayerMask mask;
    // Raycast gorece pahali bir islem. O yuzden mumkun oldugu kadar optimize etmekte fayda var. Yukaridaki
    // LayerMask ve buradaki ray uzunlugu bu optimizasyonun bir parcasi. Muhtemelen sonsuzdaki bir obje ile
    // Ilgilenmiyorsun. O yuzden ray'in uzunlugunu belirlemekte fayda var.
    [SerializeField] private float rayLength = 10;

    // agent objesinin gitmeye calistigi yere varip varmadigini kontrol ederken istedigimiz noktaya bu kadar
    // yaklasti mi diye bakacagiz. Sonundaki "Sqr" asil mesafenin karesi oldugunu anlatiyor. Bu da ileride
    // deginecegim bir optimizasyon.
    [SerializeField] private float arriveDistanceSqr = 0.01f;

    // Navmesh agent'i bir yerlerde bir sekilde tanimladigini varsayiyorum.
    [SerializeField] private Agent agent;

    private void Awake()
    {
        _cam = Camera.main;
    }

    // Turkce degisken ve metod isimleri kullanmak kotu bir pratik. Yarin obur gun
    // bir problem ile kod parcasini internete atarsan ne yazdigini kimse anlamayacak.
    // O yuzden her seyi Ingilizce yazacagim
    private void Update()
    {
        // Yaptigin uc if karsilastirmasi icinde en ucuzu Input.GetMouseButton(0)
        // O yuzden ilk once onu yapman daha mantikli. 
        if(Input.GetMouseButtonDown(0))
        {
            _ray = _cam.ScreenPointToRay(Input.mousePosition);
            if(Physics.Raycast(_ray, out _hit, rayLength, mask))
            {
                // Bir seyleri isimleri ile aramak cok cok kotu bir fikir. hem strin islemleri gorece pahali
                // hem stringi yanlis yazma ihtimalin var, hem de sahnedeki objenin ismi her zaman degisebilir.
                // Isim degisirse kodun her satirini tek tek incelemen gerekecek, ben bunu nerede kullandim diye.
                // Burada kullandigim yontemi su blog yazimda anlattim: https://www.toygel.org/Icerik/Detay/77/Sabit-Degiskenler-Icin-Ipucu
                // Bu yontem sayesinde ismi yanlis yazma ihtimalin ortadan kalkiyor, ve string'i degistirmen gerekirse
                // sadece tek bir yerden degistiriyorsun.
                if(_hit.transform.name == Constants.Strings.floor)
                {
                    agent.destination = _hit.point;
                }
            }
        }

        // Attigin script'te agent.destination ile _hit.point'i karsilastiriyordun. Bu cok anlamsiz bir karsilastirma,
        // cunku zaten yukarida _hit.point'i agent.destination'a atamistin. Yani "Floor" objesine tikladigin anda bu
        // agent.destination == _hit.point karsilastirmasi her zaman dogru donecektir. Oyle tahmin ediyorum ki sen
        // bu objenin (ya da agent'in oldugu objenin) gitmeye calistigi noktaya varip varmadigini anlamaya calisiyorsun.
        // Bunun icin agetn'in pozisyonunu kontrol etmen lazim. Pozisyonu kontrol ederken "==" kullanmak cok mantikli degil.
        // Float ile yapilan islemler bizim goremedigimiz cok cok ufak hatalar ile sonuclanabilir. Mesela inspector ve
        // Debug.Log ile baktigimizda 0 gibi gorunen rakam aslinda 0.000001 olabilir. Bu nedenle iki float arasindaki farka
        // bakip maksimum kabul edilebilir hata ile karsilastirmaliyiz. Pozisyon Vector3 olarak tutuluyor, ki o da uc tane
        // float'tan olusuyor. O yuzden iki nokta arasindaki uzakligi olcmek daha mantikli.
        
        // Burada Vector3.Distance(agent.destination, agent.transform.position) kontrolu de yapabilirdik. O zaman
        // arriveDistanceSqr (yani istedigimiz noktaya olan uzakligin karesi) yerine, direk arriveDistance (yani istedigimiz
        // noktaya olan uazkligi) kullanabilirdik. Peki bunu neden yapmadik? Cunku iki nokta arasindaki mesafe hesabi yapilirken
        // en son kare kok alinir. Kare kok almak oldukca agir bir islem. DistanceSqr metodu kare koku almadan islemi bitiriyor
        // ve elimizde istedigimiz mesafenin karesi kaliyor. Ki bu da bircok durumda kullanilabilecek bir sonuc.
        if(Vector3.DistanceSqr(agent.destination, agent.transform.position) <= arriveDistanceSqr)
        {
            Debug.Log("Arrived!");
        }
    }
}