5
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ When you say "comments", do you mean those inside methods, or also the /// <summary> comments? The latter kind is the equivalent of JavaDoc comments. \$\endgroup\$ – svick Sep 13 '14 at 20:55
  • 1
    \$\begingroup\$ I believe the names are descriptive enough. I would not document getters/setters even for a public API, unless something special deserves some explanation. But I could see that some people would want to document all public methods. \$\endgroup\$ – toto2 Sep 13 '14 at 21:29
  • 2
    \$\begingroup\$ "You write too many comments." A qualified disagree. I really like the sprint and intent. Done this way it can be generated into a usable documentation file. I'd like to see this level of code-comment coverage more often. Sadly the comments overall are very poor. For example, instead of repeating method names, how about explaining what "good" and "bad" landings are. \$\endgroup\$ – radarbob Sep 13 '14 at 22:39
  • 1
    \$\begingroup\$ Hi, sorry about the comments. The majority of these are to pop up in those little helper tooltips intellisense provides. Intended as a quick memory jog for myself and nothing more. I agree that some of them are named quite badly, and don't offer anything beyond the variable's name, which I shall fix. \$\endgroup\$ – Nick Udell Sep 14 '14 at 9:19
  • \$\begingroup\$ I dissagree, comments are very important. Of course your code and your comments need to be of high quality but this is a great start. Look at any above average framework, it has at least a ration of 1:1 code:comments. \$\endgroup\$ – Madmenyo Jan 14 '15 at 22:40
3
\$\begingroup\$
  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.

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I'll try to log on later today to finish my answer, giving help on abstracting of your threshold! Otherwise it'll be tomorrow :) \$\endgroup\$ – IEatBagels Sep 26 '14 at 19:31
  • \$\begingroup\$ Thanks! I'll admit a lot of the comments are auto-generated by ghostdoc, with little done besides trying to make them make more sense (and definitely more should be done there, particularly with the descriptions). It becomes a point where it's actually more effort to remove the backing field comments, though. I definitely agree about positioning, though! \$\endgroup\$ – Nick Udell Sep 26 '14 at 19:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.