Code Review 1

Someone recently asked for feedback on their player script. This article contains my response.

Download the original script and my refactored version here.

Here are the most important changes and my reasoning behind them:


I added the export-keyword to the constants so that they can be edited from the inspector. Not only is this helpful for team mates that may want to edit these settings, it also allows you to have different players with different settings (for multiplayer or so the player can choose between a faster character with a weak blink or a slower one with a strong blink skill).

I added the onready keyword to the screensize variable. That way the initial assignment is not performed until the ready method is called. That saves you writing an explicit ready function.

General strategy

This is important because there are two ways to avoid bugs:

  1. Write long and precise code that performs checks at runtime and that is so explicit that it allows the compiler to find bugs as well.
  2. Write short and concise code that is so easy to read that bugs become apparent right away.

GDscript is designed with the second idea in mind. For example it doesn't support static typing (you can't say that motion has to be a Vector2 for example), so the compiler can't check the data types for you. But instead it offers dynamic typing which shortens the code and puts more emphasis on the variable names.

That means that the best strategy to avoid bugs in GDscript is to write code that is short and uses very descriptive names. Any line that we can save makes the code shorter, so it takes less time to read it and you have to keep less lines in memory when trying to understand what's happening.

(This is also why I don't use "pass" at the end of functions and why I don't place an empty line at the start of a function.)

I also removed the variables "direction" and "moving" for two different reasons.

Direction: don't enable conflicting states

This variable is redundant. When you set the direction to "right" you also set $Sprite.flip_h to false. When you set it to "left" you also set $Sprite.flip_h to true. So instead of checking the direction variable we can check the value of flip_h. That way we can make sure that one bug will never occur: with a separate direction variable the player could be facing right but moving left, or it could be facing correctly but blinking in the wrong direction. In other words the state of the direction variable might not match the state of the flip_h variable. Since we don't want that to be possible let's not add code that makes it possible. :-)

Moving: prefer smaller variable scopes

This variable does not need to be a member variable. No matter which path is taken through the if-elifs at the start of physics_process the value of moving will always be changed. And since the variable is used only inside the physics_process method it's not needed later, so all we achieve by making it a member is that it now exists outside the physics_process method where it's never used. So if someone later added code and started using that variable there's a good chance he'd be doing something that wasn't anticipated, which can quickly lead to bugs. So the rule of thumb is: always use the smallest scope possible.

My first step was to move the line "var motion = Vector2()" to the start of the physics_process method, but as a second step I replaced it altogether.

Let's have a look at my physics_process method:

Summarize the class in high-level code at the top

This is basically everything the player does wrapped up in three lines of code. This is what I meant with optimizing for short and readable code. Even a "non-programmer" would be able to guess what happens when they removed the line "perform_movement" or "clamp_position_to_screen" (if they know what the term "clamp" means).

If you find that the player doesn't move correctly you can add a line like "print(input)" between lines 13 and 14. If the input is correct you know that the bug can only be inside the perform_movement-method. That way you make it easy to understand what the class does and what it doesn't do, and you apply a divide-and-conquer-strategy to your debugging.

clamp is short for min(max())

The clamp_position_to_screen-method is pretty straightforward. Using the clamp function saves you the trouble of combining a max with a min call.

Separate different tasks

First the input is retrieved. The get_input-method checks which buttons are pressed and creates a Vector2 in the range of (-1,-1) to (1,1). That vector can later be used as a factor for movement and blinking. This also spares the need for a "moving" variable, since a value of (0,0) means that the player is not moving and every other value means that it is.

The perform_movement-method applies the input variable. We could have passed the input from get_input to perform_movement by storing it in a member variable, but as I said "always use the smallest scope".

You won't find the code that actually changes the movement variable in this function because again it only lists the functions that should be run. That way it becomes much easier to follow what happens when, which is immensely helpful when your code doesn't end up doing what you were expecting.

The speed_up method again should be readable enough. Since input is a vector now I just had to scale it with ACCELERATION and add it to the motion variable. Then I used clamp again to avoid having to combine min and max.

The most important difference to the original code is that in my code getting the input, increasing the motion and clamping it to a maximum are separated. If I wanted to change the code so that there is no maximum speed anymore I would just have to remove lines 37 and 38. In your code you would have to make a more difficult modification to four lines.

To be fair in the original code it would be easier to change movement in one direction only (e.g. when moving right you should have a different top speed), but that kind of modification is much less likely.

Simplifying lerp

My code for slowing down uses the same formula as the original code, I just didn't use the lerp-function, because lerping between a value and 0 is the same as multiplying by one minus the lerp factor.

Whether this code is easier to read or not is up to personal preference, depending mostly on how familiar you are with the underlying maths. For me it seems unnecessarily complicated to lerp towards 0 since that makes half of the underlying formula meaningless. But if you are less familiar with it the keyword lerp may tell you quickly what's going on while it may take a second to understand the formula.

Replace if/else with a factor of -1/1

In line 42 you see the result of removing the moving-varialbe. I don't need that variable because I can just compare with an unmodified Vector2 to find out if any input key was pressed.

Blinking is performed in the view direction, so I need to know whether I need to add or substract BLINK_SPEED. This depends on the view direction - when the player looks to the right the view direction is (1,0), when it looks to the left it's (-1,0), so by multiplying with get_view_dir().x we get the + or - sign that we need.

flipping the sprite means changing the view direction

And this is how the view direction is generated: I just check the Sprite. If it's flipped it looks towards (-1,0), otherwise it looks towards (1,0). This solution may seem like overkill since the player can't look upwards or downwards, so the y-component will always be 0, but the term "view direction" is easy to understand, and in 2D it's to be expected that it's a Vector2, so this makes the code easier to read for most people than having a variable that stores the direction as the string "left" or "right".

And it also allows me to apply line 43 to the whole movement vector, so if you decided that it should be possible to blink in any direction you would just have to remove the two ".x"s in line 41 and return a different view direction.


I understand that this style may take a little time to get used to, but I'd argue that there are solid reasons why it helps reduce bugs and speed up the development process.