godot-code-review

Godot コードレビュー

GDScript と C# を対象とする、Godot 4.3 以降のプロジェクト向けの体系的なレビューガイドです。各チェックリストのセクションを確認し、最後にある出力テンプレートを使ってレビューの概要を作成します。

関連スキル： TDD とテストカバレッジについては godot-testing、シーンツリーのベストプラクティスについては scene-organization、パフォーマンスレビューについては godot-optimization を参照してください。

---

1. ノードとシーンのアーキテクチャ

• 各シーンの責務は単一かつ明確です（プレイヤー、敵、UI ウィジェットなど）。
• 継承チェーンは浅くします。深い extends 階層より、子ノードによるコンポジションを優先してください。
• オートロード（シングルトン）は控えめに使用します。本当にグローバルな状態だけを配置してください。
• ノード参照は直接の子ノードまでに限定し、get_parent() の連鎖は使いません。
• @onready（GDScript）または GetNode<T>()（C#）の参照先は、同じシーン内の直接の子ノードまたは名前付きパスにします。

アンチパターン — get_parent() の連鎖

# 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);
}

修正 — 代わりにシグナルを送信する

# 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 のスタイル

• 変数名と関数名には snake_case を使用します。
• class_name で宣言するクラス名には PascalCase を使用します。
• 定数には SCREAMING_SNAKE_CASE を使用します。
• すべての関数の引数と戻り値に型ヒントを付けます。
• @export 変数には明示的な型を指定します。
• シグナル宣言はファイルの先頭、変数より前に記述します。

悪い例 — 型なし

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;
}

良い例 — 型あり

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# のスタイル

• Godot のソースジェネレーターが機能するよう、ノードのスクリプトでは partial class を使用します。
• メソッドとプロパティには PascalCase、ローカル変数には camelCase を使用します。
• [Export] プロパティには PascalCase を使用します。
• [Signal] デリゲートには <EventName>EventHandler の命名パターンを使用します。
• GetNode<T>() の結果は null チェックを行うか、_Ready() でキャッシュして検証します。

// 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. パフォーマンス

• _process() または _physics_process() 内では get_node() / $NodePath を呼び出しません。必ず @onready でキャッシュします。
• 処理頻度の高い箇所では load() を呼び出しません。コンパイル時の読み込みには preload() を使うか、結果をキャッシュします。
• ノードがフレームごとの更新を必要としない場合、_process() を無効にします（set_process(false)）。
• _process() 内や短いループ内での比較には、StringName（または &"string" リテラル）を使用します。

アンチパターン — _process() 内でノードをキャッシュせずに検索する

# BAD: get_node() traverses the tree every frame
func _process(delta: float) -> void:
    get_node("HUD/HealthBar").value = health
    get_node("HUD/Label").text = str(health)

// BAD: GetNode() traverses the tree every frame
public override void _Process(double delta)
{
    GetNode<ProgressBar>("HUD/HealthBar").Value = _health;
    GetNode<Label>("HUD/Label").Text = _health.ToString();
}

修正 — @onready でキャッシュする

# GOOD: resolved once at scene load
@onready var _health_bar: ProgressBar = $HUD/HealthBar
@onready var _health_label: Label = $HUD/Label

func _process(delta: float) -> void:
    _health_bar.value = health
    _health_label.text = str(health)

// GOOD: resolved once in _Ready()
private ProgressBar _healthBar = null!;
private Label _healthLabel = null!;

public override void _Ready()
{
    _healthBar = GetNode<ProgressBar>("HUD/HealthBar");
    _healthLabel = GetNode<Label>("HUD/Label");
}

public override void _Process(double delta)
{
    _healthBar.Value = _health;
    _healthLabel.Text = _health.ToString();
}

処理頻度の高い箇所での StringName

# BAD: a String compared to a StringName is compared character by character
if animation_name == "run":
    pass

# GOOD: StringName literal — a pointer comparison
if animation_name == &"run":
    pass

// BAD: allocates a new StringName each frame
if (animationName == "run") { }

// GOOD: cache StringName as a static field
private static readonly StringName RunAnim = new("run");

public override void _Process(double delta)
{
    if (animationName == RunAnim) { }
}

---

5. 入力処理

• すべてのアクションで、ハードコードされたキー定数ではなく Input Map の名前を使用します（Project > Project Settings > Input Map）。
• UI コントロールが先にイベントを処理できるよう、_input() より _unhandled_input() を優先します。
• 継続的な移動は、_physics_process() 内で Input.get_vector() / Input.is_action_pressed() を使って処理します。
• ジャンプや射撃など、1 回限りのアクションは _unhandled_input() で処理します。

# Continuous movement — physics process
func _physics_process(delta: float) -> void:
    var direction: Vector2 = Input.get_vector(
        &"ui_left", &"ui_right", &"ui_up", &"ui_down"
    )
    velocity = direction * speed
    move_and_slide()

# Discrete action — unhandled input
func _unhandled_input(event: InputEvent) -> void:
    if event.is_action_pressed(&"jump"):
        _jump()

// Continuous movement — physics process
public override void _PhysicsProcess(double delta)
{
    Vector2 direction = Input.GetVector(
        "ui_left", "ui_right", "ui_up", "ui_down"
    );
    Velocity = direction * Speed;
    MoveAndSlide();
}

// Discrete action — unhandled input
public override void _UnhandledInput(InputEvent @event)
{
    if (@event.IsActionPressed("jump"))
    {
        Jump();
    }
}

---

6. シグナルと通信

• シグナルはツリーを上方向に伝わり（子が送信し、親または祖先が接続）、メソッド呼び出しは下方向に進みます（親が子のメソッドを呼び出します）。
• 接続は _ready() 内で確立するか、エディターで設定します。_process() 内や一度限りのコールバック内では行いません。
• ノード間に循環するシグナル依存関係がありません。
• シグナル名には、起きたことを表す過去形を使用します。

# Good signal names
signal health_changed(new_health: int)   # past tense
signal enemy_died()                       # past tense
signal item_collected(item: ItemData)     # past tense

# Bad signal names (present/imperative tense)
# signal update_health(value: int)
# signal die()
# signal collect_item(item: ItemData)

// Good signal names — past tense, EventHandler suffix
[Signal]
public delegate void HealthChangedEventHandler(int newHealth);
[Signal]
public delegate void EnemyDiedEventHandler();
[Signal]
public delegate void ItemCollectedEventHandler(ItemData item);

// Bad signal names (present/imperative tense)
// public delegate void UpdateHealthEventHandler(int value);
// public delegate void DieEventHandler();
// public delegate void CollectItemEventHandler(ItemData item);

# Parent connects to child signal in _ready()
func _ready() -> void:
    $Enemy.enemy_died.connect(_on_enemy_died)
    $Player.health_changed.connect(_on_player_health_changed)

// Parent connects to child signal in _Ready()
public override void _Ready()
{
    GetNode<Enemy>("Enemy").EnemyDied += OnEnemyDied;
    GetNode<Player>("Player").HealthChanged += OnPlayerHealthChanged;
}

---

7. リソース管理

• 編集時に判明しているリソース（シーン、テクスチャ、オーディオ）には preload() を使用し、実行時に解決するパスには load() を使用します。
• 実行時に読み込む大きなリソースやレベル固有のリソースには、フレームの停止を避けるため ResourceLoader.load_threaded_request() を使用します。
• 動的にインスタンス化したノードは、use-after-free によるクラッシュを避けるため free() ではなく queue_free() で解放します。

# Compile-time — path is validated by the editor
const BULLET_SCENE: PackedScene = preload("res://scenes/bullet.tscn")

# Runtime — path comes from data
func _load_level(path: String) -> void:
    ResourceLoader.load_threaded_request(path)

func _check_load(path: String) -> void:
    if ResourceLoader.load_threaded_get_status(path) == ResourceLoader.THREAD_LOAD_LOADED:
        var scene: PackedScene = ResourceLoader.load_threaded_get(path)
        get_tree().change_scene_to_packed(scene)

# Cleanup
func _on_enemy_died() -> void:
    queue_free()   # safe — deferred until end of frame

// Compile-time equivalent — load once in a static field or _Ready()
private static readonly PackedScene BulletScene =
    GD.Load<PackedScene>("res://scenes/bullet.tscn");

// Runtime — path comes from data
private void LoadLevel(string path)
{
    ResourceLoader.LoadThreadedRequest(path);
}

private void CheckLoad(string path)
{
    if (ResourceLoader.LoadThreadedGetStatus(path) == ResourceLoader.ThreadLoadStatus.Loaded)
    {
        var scene = ResourceLoader.LoadThreadedGet(path) as PackedScene;
        GetTree().ChangeSceneToPacked(scene);
    }
}

// Cleanup
private void OnEnemyDied()
{
    QueueFree(); // safe — deferred until end of frame
}

---

8. エラーが発生しやすいパターン

パターン	問題	対処法
queue_free() の後に await get_tree().create_timer(t).timeout を実行する（GDScript）	解放されたノードのコルーチンは通知なく破棄されます。エラーは発生せず、await の後の処理も実行されないため、そこで is_instance_valid(self) を呼び出しても実行されません	必ず実行する処理は queue_free() の前に行います。await の後には 他の ノードに対して is_instance_valid() で再確認します。ノードとともに終了させるエフェクトには create_tween() を使用します
$A/B/C/D/E のような壊れやすいノードパス	シーンツリーを再編成すると、実行時に失敗します（Node not found）	直接の子ノードとシグナルを使うようリファクタリングするか、NodePath をエクスポートします
call_deferred() をあらゆる場面で使用する	遅延呼び出しはフレームをまたぐ際の安全性を確保するためのものであり、汎用的な解決策ではありません。過剰に使うと、実際の設計上の問題が隠れてしまいます	物理スレッドとメインスレッドの境界をまたぐ場合、または呼び出しの循環を解消する場合にのみ遅延させます
_physics_process() 内で set_physics_process(true) を呼び出す	毎フレームの冗長な呼び出しとなり、CPU を無駄に消費します	処理の有効・無効を切り替えたい箇所で一度だけ呼び出します
CharacterBody2D の position を直接設定する	衝突判定を迂回してボディをテレポートさせるため、トンネリングが発生することがあります	velocity とともに move_and_slide() を使用します。position／global_position は意図的にテレポートさせる場合にのみ設定します

---

9. レビュー出力形式

レビューを提出する際は、次のテンプレートを使用してください。

## Code Review — <FileName or Feature>

### Critical
Issues that will cause bugs, crashes, or significant performance problems.

- [ ] <node/line> — <issue> — **Suggested fix:** <fix>

### Improvements
Code quality, style, or maintainability concerns that should be addressed.

- [ ] <node/line> — <issue> — **Suggested fix:** <fix>

### Positive
What the code does well — reinforce good patterns.

- <observation>

---
Reviewed against: Godot 4.3+ best practices

例

## Code Review — PlayerController.gd

### Critical
- [ ] _process() line 42 — `get_node("HUD/HealthBar")` called every frame — **Suggested fix:** Cache with `@onready var _health_bar: ProgressBar = $HUD/HealthBar`
- [ ] take_damage() line 67 — no type hints on parameter or return — **Suggested fix:** `func take_damage(amount: int) -> void:`

### Improvements
- [ ] Line 12 — signal `updateHealth` should be past tense — **Suggested fix:** Rename to `health_changed`
- [ ] Line 8 — `var speed = 200` missing type hint — **Suggested fix:** `var speed: float = 200.0`

### Positive
- Signals are declared at the top of the file
- Constants correctly use SCREAMING_SNAKE_CASE
- `queue_free()` used correctly for cleanup

---
Reviewed against: Godot 4.3+ best practices