/godot-code-review

name: godot-code-review
description: Use when reviewing GDScript or C# Godot code — checklist of best practices, common anti-patterns, and Godot-specific pitfalls

Godot Code Review

A structured review guide for Godot 4.3+ projects covering GDScript and C#. Work through each checklist section, then produce a review summary using the output template at the end.

> **Related skills:** **godot-testing** for TDD and test coverage, **scene-organization** for scene tree best practices, **godot-optimization** for performance review.

---

1. Node & Scene Architecture

• [ ] Each scene has a single, clear responsibility (player, enemy, UI widget, etc.)
• [ ] Inheritance chains are shallow — prefer composition via child nodes over deep `extends` hierarchies
• [ ] Autoloads (singletons) are used sparingly; only truly global state belongs there
• [ ] Node references traverse only to direct children — no `get_parent()` chains
• [ ] `@onready` (GDScript) or `GetNode<T>()` (C#) targets direct children or named paths within the same scene

Anti-pattern — `get_parent()` chain

# BAD: tight coupling, breaks if the tree changes
func take_damage(amount: int) -> void:
    get_parent().get_parent().get_node("HUD").update_health(health)

// BAD: tight coupling, breaks if the tree changes
public void TakeDamage(int amount)
{
    GetParent().GetParent().GetNode("HUD").Call("UpdateHealth", _health);
}

Fix — emit a signal instead

# GOOD: parent/ancestor listens; child stays decoupled
signal health_changed(new_health: int)

func take_damage(amount: int) -> void:
    health -= amount
    health_changed.emit(health)

// GOOD: parent/ancestor listens; child stays decoupled
[Signal]
public delegate void HealthChangedEventHandler(int newHealth);

public void TakeDamage(int amount)
{
    _health -= amount;
    EmitSignal(SignalName.HealthChanged, _health);
}

---

2. GDScript Style

• [ ] Variables and functions use `snake_case`
• [ ] Class names declared with `class_name` use `PascalCase`
• [ ] Constants use `SCREAMING_SNAKE_CASE`
• [ ] All function parameters and return types carry type hints
• [ ] `@export` variables include an explicit type
• [ ] Signal declarations appear at the top of the file, before variables

Bad — untyped

var speed = 200
var health = 100

func move(direction):
    position += direction * speed

func heal(amount):
    health += amount
    return health

// BAD: no explicit types, weak contracts
float speed = 200;
int health = 100;

public void Move(object direction)
{
    Position += (Vector2)direction * speed;
}

public object Heal(object amount)
{
    health += (int)amount;
    return health;
}

Good — typed

class_name PlayerController
extends CharacterBody2D

signal health_changed(new_health: int)
signal player_died()

const MAX_HEALTH: int = 100
const BASE_SPEED: float = 200.0

@export var speed: float = BASE_SPEED
@export var max_health: int = MAX_HEALTH

var health: int = max_health

func move(direction: Vector2) -> void:
    velocity = direction * speed
    move_and_slide()

func heal(amount: int) -> int:
    health = mini(health + amount, max_health)
    health_changed.emit(health)
    return health

// GOOD: strongly typed, proper C# conventions
public partial class PlayerController : CharacterBody2D
{
    [Signal]
    public delegate void HealthChangedEventHandler(int newHealth);
    [Signal]
    public delegate void PlayerDiedEventHandler();

    private const int MaxHealth = 100;
    private const float BaseSpeed = 200f;

    [Export] public float Speed { get; set; } = BaseSpeed;
    [Export] public int MaxHp { get; set; } = MaxHealth;

    private int _health;

    public override void _Ready()
    {
        _health = MaxHp;
    }

    public void Move(Vector2 direction)
    {
        Velocity = direction * Speed;
        MoveAndSlide();
    }

    public int Heal(int amount)
    {
        _health = Mathf.Min(_health + amount, MaxHp);
        EmitSignal(SignalName.HealthChanged, _health);
        return _health;
    }
}

---

3. C# Style

• [ ] Node scripts use `partial class` to allow Godot source generators to work
• [ ] Methods and properties use `PascalCase`; local variables use `camelCase`
• [ ] `[Export]` properties use `PascalCase`
• [ ] `[Signal]` delegates follow the `<EventName>EventHandler` naming pattern
• [ ] `GetNode<T>()` results are null-checked or cached in `_Ready()` and validated

// GOOD
public partial class PlayerController : CharacterBody2D
{
    [Signal]
    public delegate void HealthChangedEventHandler(int newHealth);

    [Export] public float Speed { get; set; } = 200f;
    [Export] public int MaxHealth { get; set; } = 100;

    private int _health;
    private AnimationPlayer _animationPlayer = null!;

    public override void _Ready()
    {
        _animationPlayer = GetNode<AnimationPlayer>("AnimationPlayer");
        // Validate at startup rather than silently failing later
        if (_animationPlayer is null)
            GD.PushError("AnimationPlayer node not found on PlayerController");

        _health = MaxHealth;
    }

    public void TakeDamage(int amount)
    {
        _health = Mathf.Max(_health - amount, 0);
        EmitSignal(SignalName.HealthChanged, _health);
    }
}

---

4. Performance

• [ ] `get_node()` / `$NodePath` is never called inside `_process()` or `_physics_process()` — always cache with `@onready`
• [ ] `load()` is not called in hot paths — use `preload()` for compile-time loading or cache the result
• [ ] `_process()` is disabled (`set_process(false)`) when the node does not need per-frame updates
• [ ] `StringName` (or `&"string"` literal) is used for comparisons inside `_process()` or tight loops

Anti-pattern — uncached node lookup in `_process()`

# BAD: get_node() traverses the tree every frame
func _process(delta: float) -> void