Determining vehicle jump landing quality

Question

I am writing a game where a car drives and makes jumps. When a jump is landed, the player is rewarded if they land all four wheels either at the same time, or near to the same time. If they don't, they are penalised.

I am concerned my solution is not well abstracted (although I am open to all topics of feedback), and particularly that it will not be very extensible in the future should I come to add additional things to check for (such as bonuses for good air time, or tricks) on landing.

using System.Collections.Generic;
using System.Linq;
using UnityEngine;

/// <summary>
/// Checks if the car has made a good landing or not.
/// </summary>
[RequireComponent(typeof(CollisionEvent))] //Unity engine code to ensure a CollisionEvent is always attached to the same object as this script.
public class GoodLandingChecker : MonoBehaviour
{

    /// <summary>
    /// Gets or sets the bad landing threshold.
    /// When a bad landing occurs, the player is penalised.
    /// </summary>
    /// <value>
    /// The bad landing threshold.
    /// </value>
    public float BadLandingThreshold
    {
        get { return badLandingThreshold; }
        set { badLandingThreshold = value; }
    }

    /// <summary>
    /// Gets or sets the good landing threshold.
    /// When a good landing occurs, the player is rewarded.
    /// </summary>
    /// <value>
    /// The good landing threshold.
    /// </value>
    public float GoodLandingThreshold
    {
        get { return goodLandingThreshold; }
        set { goodLandingThreshold = value; }
    }

    /// <summary>
    /// Gets or sets the great landing threshold.
    /// When a great landing occurs, the player is rewarded greatly.
    /// </summary>
    /// <value>
    /// The great landing threshold.
    /// </value>
    public float GreatLandingThreshold
    {
        get { return greatLandingThreshold; }
        set { greatLandingThreshold = value; }
    }

    /// <summary>
    /// Gets or sets the minimum flying time before a landing will be considered.
    /// </summary>
    /// <value>
    /// The minimum flying time.
    /// </value>
    public float MinimumFlyingTime
    {
        get { return minFlyingTime; }
        set { minFlyingTime = value; }
    }

    /// <summary>
    /// Gets or sets the wheels used to calculate when landings happen.
    /// </summary>
    /// <value>
    /// The wheels.
    /// </value>
    public List<Transform> Wheels
    {
        get { return wheels; }
        set { wheels = value; }
    }

    /// <summary>
    /// The bad landing threshold
    /// </summary>
    [SerializeField] //Unity code to make this private field show up in Unity's
    //"Inspector" where its value is set by the designer. This is used because the
    //Inspector cannot display properties.
    private float badLandingThreshold;

    /// <summary>
    /// Flying checker is a separate class that simply checks if every wheel is touching the ground.
    /// </summary>
    [SerializeField]
    private FlyingChecker flyingChecker;

    /// <summary>
    /// The time spent in the air since the last landing.
    /// </summary>
    private float flyingTime = 0f;

    /// <summary>
    /// The good landing threshold
    /// </summary>
    [SerializeField]
    private float goodLandingThreshold;

    /// <summary>
    /// The great landing threshold
    /// </summary>
    [SerializeField]
    private float greatLandingThreshold;

    /// <summary>
    /// Whether the car is flying right now or not.
    /// </summary>
    private bool isFlying = false;

    /// <summary>
    /// The minimum flying time before a landing will be considered.
    /// </summary>
    [SerializeField]
    private float minFlyingTime;

    /// <summary>
    /// The wheels
    /// </summary>
    [SerializeField]
    private List<Transform> wheels;

    /// <summary>
    /// Checks the angle of the car.
    /// If it's fairly flat, a good landing is awarded.
    /// </summary>
    /// <param name="wheel">The wheel that landed.</param>
    public void WheelLanded(Transform wheel)
    {
        if (isFlying && flyingTime >= minFlyingTime)
        {
            isFlying = false;

            //Get height of other wheel
            float wheelHeightDifference = Wheels.Where(x => x != wheel).Sum(x => Mathf.Abs(x.position.y - wheel.position.y));
            if (wheelHeightDifference <= GreatLandingThreshold)
            {
                Debug.Log("Great!");
                //Give the car a *huge* forward push
                rigidbody.AddForce(transform.forward * 500000);
            }
            else if (wheelHeightDifference <= GoodLandingThreshold)
            {
                Debug.Log("Good");
                //Give the car a forward push
                rigidbody.AddForce(transform.forward * 200000);
            }
            else if (wheelHeightDifference >= BadLandingThreshold)
            {
                Debug.Log("Awful!");
                //Give the car a backward push
                rigidbody.AddForce(-transform.forward * 200000);
            }
            else
            {
                Debug.Log("Ok");
                //Normal landing, ignore
            }
        }
        flyingTime = 0f;
    }

    /// <summary>
    /// Handles the CollisionEntered event of the GoodLandingChecker control.
    /// This occurs when something has hit something.
    /// </summary>
    /// <param name="sender">The source of the event.</param>
    /// <param name="e">The <see cref="Game.View.CollisionEventArgs"/> instance containing the event data.</param>
    private void GoodLandingChecker_CollisionEntered(object sender, Game.View.CollisionEventArgs e)
    {
        //Check whether any wheels hit the ground.
        var wheel = Wheels.FirstOrDefault(w => e.Collision.contacts.Select(x => x.thisCollider).Any(x => w.collider == x));
        if (wheel != null)
        {
            WheelLanded(wheel);
        }
    }

    /// <summary>
    /// Called when this object is destroyed by Unity.
    /// </summary>
    private void OnDestroy()
    {
        GetComponent<CollisionEvent>().CollisionEntered -= GoodLandingChecker_CollisionEntered;
    }

    /// <summary>
    /// Called by Unity when the game starts.
    /// </summary>
    private void Start()
    {
        GetComponent<CollisionEvent>().CollisionEntered += GoodLandingChecker_CollisionEntered;
    }

    /// <summary>
    /// Called by Unity every frame.
    /// </summary>
    private void Update()
    {
        //Flying checker is a separate class that simply checks if every wheel is touching the ground
        isFlying = isFlying || flyingChecker.IsFlying();
        if (isFlying)
        {
            flyingTime += Time.deltaTime;
        }
    }
}

Answer 1

First, I am not a C# programmer, but a Java programmer. So maybe some of what I write here is wrong for C#.

• You write too many comments. Let the code speak for itself.

• GoodLandingChecker_CollisionEntered Are you supposed to use underscore notation in C#?

• You can probably find a better name for WheelLanded. Either OnWheelLanded if it's a callback, or something which is a verb otherwise, ie analyzeLanding.

• I would split WheelLanded in a series of methods:

  1. a method that returns the height difference
  2. a method that transform the numerical height into an enum
  3. one (or many) delegates that register to the get the enum result.

  For example, the debugging print out and the added forward motion would be two separate delegates.

• I am not sure if FlyingChecker should be separated from this class. Maybe you could have the method that determines what kind of landing (enum) it is in FlyingChecker, but have the delegates that act on that information being defined elsewhere.

Answer 2

1. I think the xml doc comments are ok although in general mostly I'm dubious about documenting the backing fields as well as the properties. I typically like to keep them together like this:

   [SerializeField]
   private int _GreatLandingThreshold;
   public int GreatLandingThreshold
   {
       get { return _GreatLandingThreshold; }
       set { _GreatLandingThreshold = value; }
   }
   

2. As the standard naming convention for local variables and parameters is camelCase an established practice is to prefix private class members with _ although using this. instead works as well. Matter of personal preference I guess.

3. You can remove some in-code comments by creating helper methods which state what they are doing. For example instead of this:

   //Get height of other wheel
   float wheelHeightDifference = Wheels.Where(x => x != thisWheel).Sum(x => Mathf.Abs(x.position.y - thisWheel.position.y));
   

   extract it into a helper method and call it:

   private float GetHeighOfOtherWheel(Transform thisWheel)
   {
       return Wheels.Where(x => x != thisWheel).Sum(x => Mathf.Abs(x.position.y - thisWheel.position.y));
   }
   

   No need for a comment anymore. Similar for the code which gives the car a push.

Answer 3

I think there's too much comments in your code. The summary xml doc of your property might stay, but I'd remove the value part of it, for all your properties. And you should remove the xml on your private fields, since they are private and a copy/paste of your properties.

Also you should keep your private fields on top of the class, or close to the property it is linked to, it makes it easier to follow your code.