GPD Pong: Code Review

Unity Engine
1

Hi there!

I would like to have someone please review my code for a game I made last September: GPD Pong, published on Newgrounds and Kongregate. It’s a chance for me to keep learning more about the Unity API and change my coding style. I want to get university co-ops in the gaming industry, and the employers of companies I’m applying to are very concerned about the way the code is written and what it’s functionality.

The code for the game is hosted in this public GitHub repository:

I think you can clone the repo or just download it entirely if you don’t want to read through it while on GitHub.

2

You’ve got a lot of magic strings in various methods. Pull those out to a constants definitions file, so that changes can be localized. For example, if one of the names of your tags changes, you only have to change it in one place.

I notice that you’re using Time.deltaTime inside of FixedUpdate methods. For example, you’re setting the position of the ball inside of a FixedUpdate with Time.deltaTime. This is weird. It’d be better to use Time.fixedDeltaTime when inside of the fixedupdate method. Better yet is to just set the velocity of the rigidbody, and let the physics engine handle translation for you (see “Moving and Rotating Rigidbodies” at Best Practices For Rigid Bodies in Unity – Digital Opus).

You’re doing a lot of processing directly inside of unity event methods. This isn’t bad, per se. I caution against it. I recommend extracting the code inside of unity methods into their own named methods that describe what they do. For example, Ball.OnCollisionEnter could be broken up into two smaller methods such as HandleCollisionWithWall and HandleCollisionWithGoal (which could themselves be broken up a little bit more).

I highly recommend a copy of Bob Martin’s Clean Code.

3

This book? https://www.amazon.ca/Clean-Code-Handbook-Software-Craftsmanship/dp/0132350882

4

Just a little note:
Unity takes care of this internally and returns the corresponding delta time for Update / FixedUpdate.

5

Here are some things that I noticed and that I would improve.

Conventions/Consistency:

  1. sometimes you start methods with capital letters, then again with lower ones (C# conventions often use PascalCase for methods)
  2. some of the if statements are a pain to read due to long condition chains, a descriptive method would improve readability
  3. Stay consistent with attribute placement (in front/above)
  4. It’s best to always provide the access modifier
  5. …

I would just refactor the names using a common convention. Usually you’ll be told which conventions to use in a company, but it cannot be bad to show that you stay consistent once you decided to use one.

Improvements:
5) consider to encapsulate fields that are not readonly/const
6) That byte array with the goals in the ApplicationManager is as slight and simple as it could be, but IMO it’d be much better to give those goals some kind of type / identity even if they only represent a primitive type internally. This improves readability when working with it, allows to achieve better maintainability and improves extensibility.
Noone would want to look up what

goalsToAchieve[2]

was again. Any change to such as list could be a code- or logic-breaking change. (I know that the game is finished and won’t change, but this applies generally and is not recommended. Unless you’re doing some extreme native performance optimizations in a appropriate low-level language.

  1. Don’t do this:
new void Start()
{
    base.Start();
    [...]
}

The ‘new’ keyword basically says you want to re-define the method with that name. This breaks polymorphism at some point. The proper way would be:

protected override void Start()
{
    base.Start();
    [...]
}

  1. Some of your classes are meant to be singletons and always implement the same (in your case short) pattern. Think about a way to implement that pattern in one place to be re-usable and maintainable.
    Also, as you destroy your singleton, you should prevent further initialization due to the way Destroy() works. The Awake method will not cancel but run to the end. This may introduce nasty bugs.

  2. Document your code using

/// <summary>
/// [Add whatever Foo() does]
/// </summary>
/// <returns> [add some information here] </returns>
public [return type] Foo() { }

This is really important for companies, teamwork and for yourself when you come back to code written some time ago.

  1. As already mentioned by @kru , you might want to split up some of the methods. Follow the Single Responsibility Principle.

  2. It’s not a big project, but that also allows you to easily demonstrate that you can decouple the logic from the presentation. Depends how much you want to impress :stuck_out_tongue: