Clean code example

Hi. I've been using Unity for a couple of months now and since I have no previous experience in coding then I've just gone with simple solutions which I've picked up from tutorials and which work for me. Recently I've read about clean code, so I thought maybe someone could give me advice with my simple code, like what to change and what not to use or to etc. Following is a little script form my infinite jumper game which controls the game over sequence. Any advice is appreciated.

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4. using UnityEngine.UI;
5.  
6. public class Game_Over_Controller : MonoBehaviour
7. {
8.     private GameObject[] gameOverObjects;
9.  
10.     public static bool isGameOver;
11.  
12.     [SerializeField] private Transform playerTransform;
13.     [SerializeField] private float fallDistanceToGameOver;
14.  
15.     private float playerHighestPoint;
16.  
17.     [SerializeField] private Text currentScoreText;
18.     [SerializeField] private Text highscoreText;
19.  
20.  
21.     // Start is called before the first frame update
22.     void Start()
23.     {
24.         isGameOver = false;
25.         gameOverObjects = GameObject.FindGameObjectsWithTag("Game_Over_Objects");
26.         HideGameOverObjects();
27.         playerHighestPoint = playerTransform.position.y;              
28.     }
29.  
30.     // Update is called once per frame
31.     void Update()
32.     {
33.         GameOver();
34.     }
35.  
36.     private void HideGameOverObjects()
37.     {
38.         foreach (GameObject g in gameOverObjects)
39.         {
40.             g.SetActive(false);
41.         }
42.     }
43.  
44.     private void ShowGameOverObjects()
45.     {
46.         foreach (GameObject g in gameOverObjects)
47.         {
48.             g.SetActive(true);
49.         }
50.     }
51.  
52.     private void GameOver()
53.     {    
54.         if (playerHighestPoint < playerTransform.position.y) playerHighestPoint = playerTransform.position.y;
55.  
56.         if (!isGameOver && playerHighestPoint - playerTransform.position.y > fallDistanceToGameOver)
57.         {
58.             isGameOver = true;
59.             DisplayScore();
60.             ShowGameOverObjects();
61.             Time.timeScale = 0;
62.         }      
63.     }
64.  
65.     private void DisplayScore()
66.     {
67.         currentScoreText.text = "Height: " + Score_Controller.currentScore + " m";
68.         highscoreText.text = "Personal Best: " + Score_Controller.highscore + " m";
69.     }
70.  
71. }
72.  

 

If it works well enough, then it works.

If you want to noodle it to be cleaner, you are moving into the realm of speculative optimization, which is a) unnecessary, b) potentially harmful, and c) won't make your game better (if it already works).

Furthermore, one guy's "clean" is another guy's "ugly," so you won't get a straight answer across the board here. I personally have no problem with any code at all as long as it is functioning as-advertised. When it starts to fall apart and misbehave, then it is possible to initiate an investigation into what might be a better approach. Lacking a clear failure or wanting, I can't really offer any advice on it.

 

That code looks pretty good to me. I can tell at a glance what's going on, which is the most important thing. The only thing I'd change would be removing the underscores from the class names (they're not really standard, and I dislike underscores in names generally), but as @Kurt-Dekker says, this is very much a personal taste thing.

That said, I think that asking this question is indeed a good thing, because I don't agree that

is universally true. I could easily take the posted code, rewrite some names without changing the functionality, and make it objectively difficult to read and understand. I could make the variable names gibberish, make them describe things that they aren't, or going subtler, I could use UpperCase for variable names and camelCase for captions (the inverse of established standards, particular in the Unity codebase), to give three examples.

There's a lot of personal tastes in terms of what the best code style practices are, but just because there are a lot of right answers, it doesn't mean that there's no such thing as a wrong answer. So yeah, it is good to ask this sort of thing.

 

Ah well, you must be lucky... I just get yelled at when I ask this this sort of thing and there are still pending tickets waiting for me to fix.

 

Here's some notes on your code:
First off, static fields are generally considered Bad, although it's a sin we all wind up being guilty of sometimes; global state creates all kinds of problems. Public fields are also considered Bad, because they break the principle of encapsulation. So a public static field is doubly a sin; basically anything can change that field at any point in time and easily completely break behavior.

Another thing to keep in mind is DRY, which just stands for Don't Repeat Yourself. If you have two very similar chunks of code, you can almost always replace them with one piece of code. For example, your HideGameOverObjects and ShowGameOverObjects methods are nearly exactly the same, and can easily be replaced with one method:

Code (CSharp):

1. private void ToggleGameOverObjects(bool newState)
2. {
3.     foreach (GameObject g in gameOverObjects)
4.     {
5.         g.SetActive(newState);
6.     }
7. }

 

It is a good question to ask and I wish I had learned a bunch of coding standards before jumping into projects that just built up on top of me and became unmanageable.
Everyone on these forums tells you to start small and learn through experience, but there are loads of people out there who have already been through the struggles come up with standards that will help you write "clean" (robust, readable, performant, logical, testable, extendible, loosely-coupled) code.
Of course, you do still need to learn through experience with Unity, because there are a lot of different things to learn, and half of it you'll probably never touch.

Here are some of my favorite coding principles which have helped me immensely in recent projects:

Separation of responsibility:
One class should do one thing. This makes it clearer what each class does, it allows you to reuse it for that one thing in different situations and it makes it easier to test.
I would even advise that you dont put any methods in your monobehaviours, just create a class that does that thing you want, and call that object from your monobehaviour in your start and update methods.

YAGNI:
You aint gonna need it. This comes down to designing your game before developing it, and not adding things that you aren't sure you'll use. If you think "My character can jump, but maybe later i'll add a double-jump, so I'll make my code support double-jumps" just don't do that, stick with the single jump and add double-jump later if it's actually a requirement.

TDD:
Test driven development. This is a very controversial subject because testing can be extremely time consuming, especially in game development since user input is very difficult to test.
It basically means that you write a test for your code before actually writing the code. This forces you to write testable code and clean code, because the cleaner and more testable your classes are, the easier they are to use.
I've recently started writing tests fir my code and although it slows down development initially, I've noticed I spend significantly less time fixing little annoying bugs, and when I need to change something I'm confident that I haven't broken anything else in a matter of seconds because my tests will tell me.

Anyway I'm probably recycling things other people have said and getting it all wrong and missing things out. But there are plenty of resources out there which will help you learn these things.

Check out the SOLID principles to get started:
https://alanbarber.com/post/solid-p...les-of-objectoriented-programming-and-design/

Hopefully, that helps. Sorry for the novel!

 

Before you take any advice, read through your code. Think about the names of your methods and variables. Do they describe what those things do/are well?

After that, I'd first recommend doing what @Dameon_ has already pointed out. Once that's done. the biggest issue when reading your code is this:

Code (csharp):

1. void Update()
2. {
3.     GameOver();
4. }

Which looks super-weird! You game over every frame? That doesn't sound like a very fun game!

When we look at GameOver(), what the function is actually doing is checking for a gameover, and then if the requirements for a gameover is met, doing a game over routine. So the method name needs improving, for readability.

There's a bunch of ways you could change the code. Pick the one you think looks nicest.
1: Just rename the GameOver method to CheckForGameOver, and be done with it.

2: Move the entire GameOver method into Update. It's pretty obvious that the Update method of the Game_Over_Controller would be controlling the game over, so the "call single method" thing there isn't really necessary.

3: Break the GameOver stuff up into more named parts that describe what they do:

Code (csharp):

1.  
2. void Update()
3. {
4.     if (isGameOver)
5.         return;
6.  
7.     UpdatePlayersHighestPoint();
8.     if (ShouldGameOver())
9.         GameOver();
10. }

 

I strongly advise you not to try using your new knoweleges you obtained somewhere and your expirience on code what is already written and working. This knowledge is good when you writing new code, but if old working, leave it as is. If you live by past, you have no future. I mean, you are not creating anything new while rewriting old code, what works.

 

Not sure why people keep telling OP not to improve their skills, there are other important considerations besides whether code works. Readability, cohesion, coupling, and performance are valid concerns, among other things. Unless you're some god-like coder who writes everything perfectly the first time, you should be refactoring a class at least once after writing it.

Regardless, OP is clearly asking so they can learn about how it should be written, and it's silly to tell them not to learn to do things better.

 

I'm glad to hear that you are interested in writing clean code. Getting good at writing easy-to-understand code can be hugely beneficial, especially when you're working with other people and in larger projects.

In addition to everything others have already pointed out, here are some of my suggestions for improvements:

1. The name "gameOverObjects" is a little bit on the generic side. Something like "gameOverScreenUIObjects" or "activeOnlyWhenGameIsOver" would give a better understanding about what the field is all about. It's a bit tough one to name well since it also happens to be an array, so using a plural for the name would also be good.

2. It does not really affect readability in such a short script, but for the benefit of larger scripts you might want to be consistent in how you order your class members. Consistently place static members before instance members, public members before private members, fields before methods and so on. Execution order should also be taken into consideration (Start comes before Update etc.) so it is intuitive to read, like you already have done in your code.

3. While the risk factor here is pretty miniscule, as a general pattern initializing variables inside the Start function is a little bit unsafe, because other scripts might have already read or changed the contents of these variables before the Start function gets invoked. Since in this example there are no other public class members besides isGameOver, it is not really a problem. But if for example you were to add a public method a month later that referenced gameOverObjects, it could throw an expection.

4. It is also perhaps a little bit strange behaviour that the GameController reacts to isGameOver being set to true after a delay (only during the next Update call). An external system could set the value to true, and incorrectly assume that it gets applied immediately. Again, probably not a big deal in this particular instance, but as a general rule of thumb avoiding unexpected (and unnecessary) delays in effects like this could save you from many bugs in the future.

More about naming things well:
https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/general-naming-conventions

More about class member order conventions:
https://stackoverflow.com/a/310967

 

Thank you all for your feedback. I got many great tips from that and some of them I've already implemented into my code.
There was a discussion about simple coding vs clean and I just wanted to say that I'm interested in clean coding because my first project got really messy so updating the code later on was a real pain, it took a while to understand what I had already done. I started out with a similar game over routine script and it ended up doing much more than it was meant to. Against the odds I still managed to release this into the wilderness of Play Games store though. So with my new project I decided to put some more sense into it or at least try.
Anyways back to your feedback.

• DRY - Read about it before posting the initial thread and also thought that showing and hiding game over object should be toggled and thanks to Dameon_'s advice made the change. I used isGameOver boolean because game over objects should be visible only, when the game is over.
• Naming and organizing - Also did some renaming and planning to separate the game over routine and checking whether the game is over. Definitely will lose the underscores.
• Static fields - Picked that up from a tutorial, but when using static fields is considered a bad practice, then what would be the preferred on?
• Classes - One class should do one thing. How to determine the size of that one thing? For example should there be a different class for something in my posted code?

All in all thanks again for your feedback.

 

In terms of respecting the single responsibility principle, I think that the fact that your GameOverController also handles updating the display score is the thing that sticks out the most. It's not very intuitive that said responsibility should fall under the GameObjectController.

Fall distance detection makes a bit more sense to be included here, since it's directly related to the game over state being triggered, but it could also be better separated into its own class.


You could modularize things even further very naturally by relying more on events. The GameOverController could be a really simple class that is just used to specify when the game is over, and then everything else takes place in other classes in reaction to the GameOverController.OnGameOver event being broadcasted. But your example class is so short that it's very easy to read as it is, so there wouldn't be much benefit to be gained for doing something like that here.


In practice, at least in my case, I don't usually really think about refactoring classes into multiple smaller ones until the line count extends into the hundreds. When the line count hits a thousand I usually start feeling a strong sense of guilt inside.

In terms of pure intuitiveness, you should always think about what functionality would make sense to be included inside a given class. E.g., what if you were to return to your project after a year, and wanted to find the class responsible for updating the displayed score. Would you think to look for it inside a class named GameOverController?