spacetris/CODE_ANALYSIS.md

Tetris SDL3 - Code Analysis & Best Practices Review

Generated: 2025-12-03
Project: Tetris Game (SDL3)

---

📊 Executive Summary

Your Tetris project is well-structured and follows many modern C++ best practices. The codebase demonstrates:

• ✅ Clean separation of concerns with a state-based architecture
• ✅ Modern C++20 features and RAII patterns
• ✅ Centralized configuration management
• ✅ Proper dependency management via vcpkg
• ✅ Good documentation and code organization

However, there are opportunities for improvement in areas like memory management, error handling, and code duplication.

---

🎯 Strengths

1. Architecture & Design Patterns

• State Pattern Implementation: Clean state management with MenuState, PlayingState, OptionsState, LevelSelectorState, and LoadingState
• Separation of Concerns: Game logic (Game.cpp), rendering (GameRenderer, UIRenderer), audio (Audio, SoundEffect), and persistence (Scores) are well-separated
• Centralized Configuration: Config.h provides a single source of truth for constants, eliminating magic numbers
• Service Locator Pattern: StateContext acts as a dependency injection container

2. Modern C++ Practices

• C++20 Standard: Using modern features like std::filesystem, std::jthread
• RAII: Proper resource management with smart pointers and automatic cleanup
• Type Safety: Strong typing with enums (PieceType, AppState, LevelBackgroundPhase)
• Const Correctness: Good use of const methods and references

3. Code Organization

src/
├── audio/          # Audio system (music, sound effects)
├── core/           # Core systems (state management, settings, global state)
├── gameplay/       # Game logic (Tetris mechanics, effects)
├── graphics/       # Rendering (UI, game renderer, effects)
├── persistence/    # Score management
├── states/         # State implementations
└── utils/          # Utilities

This structure is logical and easy to navigate.

4. Build System

• CMake: Modern CMake with proper target configuration
• vcpkg: Excellent dependency management
• Cross-platform: Support for Windows and macOS
• Testing: Catch2 integration for unit tests

---

⚠️ Areas for Improvement

1. Memory Management Issues

Problem: Raw Pointer Usage

Location: MenuState.h, main.cpp

// MenuState.h (lines 17-21)
SDL_Texture* playIcon = nullptr;
SDL_Texture* levelIcon = nullptr;
SDL_Texture* optionsIcon = nullptr;
SDL_Texture* exitIcon = nullptr;

Issue: Raw pointers to SDL resources without proper cleanup in all code paths.

Recommendation:

// Create a smart pointer wrapper for SDL_Texture
struct SDL_TextureDeleter {
    void operator()(SDL_Texture* tex) const {
        if (tex) SDL_DestroyTexture(tex);
    }
};
using SDL_TexturePtr = std::unique_ptr<SDL_Texture, SDL_TextureDeleter>;

// Usage in MenuState.h
private:
    SDL_TexturePtr playIcon;
    SDL_TexturePtr levelIcon;
    SDL_TexturePtr optionsIcon;
    SDL_TexturePtr exitIcon;

Benefits:

• Automatic cleanup
• Exception safety
• No manual memory management
• Clear ownership semantics

---

2. Error Handling

Problem: Inconsistent Error Handling

Location: main.cpp (lines 86-114)

static SDL_Texture* loadTextureFromImage(SDL_Renderer* renderer, const std::string& path, int* outW = nullptr, int* outH = nullptr) {
    if (!renderer) {
        return nullptr;  // Silent failure
    }
    
    SDL_Surface* surface = IMG_Load(resolvedPath.c_str());
    if (!surface) {
        SDL_LogError(...);  // Logs but returns nullptr
        return nullptr;
    }
    // ...
}

Issues:

• Silent failures make debugging difficult
• Callers must check for nullptr (easy to forget)
• No way to distinguish between different error types

Recommendation:

#include <expected>  // C++23, or use tl::expected for C++20

struct TextureLoadError {
    std::string message;
    std::string path;
};

std::expected<SDL_TexturePtr, TextureLoadError> 
loadTextureFromImage(SDL_Renderer* renderer, const std::string& path, 
                     int* outW = nullptr, int* outH = nullptr) {
    if (!renderer) {
        return std::unexpected(TextureLoadError{
            "Renderer is null", path
        });
    }
    
    const std::string resolvedPath = AssetPath::resolveImagePath(path);
    SDL_Surface* surface = IMG_Load(resolvedPath.c_str());
    if (!surface) {
        return std::unexpected(TextureLoadError{
            std::string("Failed to load: ") + SDL_GetError(),
            resolvedPath
        });
    }
    
    // ... success case
    return SDL_TexturePtr(texture);
}

// Usage:
auto result = loadTextureFromImage(renderer, "path.png");
if (result) {
    // Use result.value()
} else {
    SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, 
                 "Failed to load %s: %s", 
                 result.error().path.c_str(),
                 result.error().message.c_str());
}

---

3. Code Duplication

Problem: Repeated Patterns

Location: MenuState.cpp, PlayingState.cpp, OptionsState.cpp

Similar lambda patterns for exit popup handling:

auto setExitSelection = [&](int value) {
    if (ctx.exitPopupSelectedButton) {
        *ctx.exitPopupSelectedButton = value;
    }
};

auto getExitSelection = [&]() -> int {
    return ctx.exitPopupSelectedButton ? *ctx.exitPopupSelectedButton : 1;
};

Recommendation: Create a helper class in StateContext:

// StateContext.h
class ExitPopupHelper {
public:
    ExitPopupHelper(int* selectedButton, bool* showPopup)
        : m_selectedButton(selectedButton), m_showPopup(showPopup) {}
    
    void setSelection(int value) {
        if (m_selectedButton) *m_selectedButton = value;
    }
    
    int getSelection() const {
        return m_selectedButton ? *m_selectedButton : 1;
    }
    
    void show() {
        if (m_showPopup) *m_showPopup = true;
    }
    
    void hide() {
        if (m_showPopup) *m_showPopup = false;
    }
    
    bool isVisible() const {
        return m_showPopup && *m_showPopup;
    }

private:
    int* m_selectedButton;
    bool* m_showPopup;
};

// Usage in states:
ExitPopupHelper exitPopup(ctx.exitPopupSelectedButton, ctx.showExitConfirmPopup);
exitPopup.setSelection(0);
if (exitPopup.isVisible()) { ... }

---

4. Magic Numbers

Problem: Some Magic Numbers Still Present

Location: MenuState.cpp (lines 269-273)

float btnW = 200.0f; // Fixed width to match background buttons
float btnH = 70.0f;  // Fixed height to match background buttons
float btnX = LOGICAL_W * 0.5f + contentOffsetX;
float btnY = LOGICAL_H * 0.865f + contentOffsetY;

Recommendation: Add to Config.h:

namespace Config::UI {
    constexpr float MENU_BUTTON_WIDTH = 200.0f;
    constexpr float MENU_BUTTON_HEIGHT = 70.0f;
    constexpr float MENU_BUTTON_Y_FRACTION = 0.865f;
    constexpr float MENU_BUTTON_SPACING = 210.0f;
}

---

5. File I/O for Debugging

Problem: Debug Logging to Files

Location: MenuState.cpp (lines 182-184, 195-203, etc.)

FILE* f = fopen("tetris_trace.log", "a"); 
if (f) { 
    fprintf(f, "MenuState::render entry\n"); 
    fclose(f); 
}

Issues:

• File handles not checked properly
• No error handling
• Performance overhead in production
• Should use proper logging framework

Recommendation:

// Create a simple logger utility
// src/utils/Logger.h
#pragma once
#include <string>
#include <fstream>
#include <mutex>

class Logger {
public:
    enum class Level { TRACE, DEBUG, INFO, WARN, ERROR };
    
    static Logger& instance();
    
    void setLevel(Level level) { m_level = level; }
    void setFile(const std::string& path);
    
    template<typename... Args>
    void trace(const char* fmt, Args... args) {
        log(Level::TRACE, fmt, args...);
    }
    
    template<typename... Args>
    void debug(const char* fmt, Args... args) {
        log(Level::DEBUG, fmt, args...);
    }
    
private:
    Logger() = default;
    
    template<typename... Args>
    void log(Level level, const char* fmt, Args... args);
    
    Level m_level = Level::INFO;
    std::ofstream m_file;
    std::mutex m_mutex;
};

// Usage:
#ifdef DEBUG
    Logger::instance().trace("MenuState::render entry");
#endif

---

6. Const Correctness

Problem: Missing const in Some Places

Location: StateContext and various state methods

Recommendation:

// State.h
class State {
public:
    virtual void render(SDL_Renderer* renderer, float logicalScale, 
                       SDL_Rect logicalVP) const = 0;  // Add const
    // Render shouldn't modify state
};

---

7. Thread Safety

Problem: Potential Race Conditions

Location: Audio.cpp - Background loading

Current:

std::vector<AudioTrack> tracks; 
std::mutex tracksMutex;

Recommendation:

• Document thread safety guarantees
• Use std::shared_mutex for read-heavy operations
• Consider using lock-free data structures for performance-critical paths

// Audio.h
class Audio {
private:
    std::vector<AudioTrack> tracks;
    mutable std::shared_mutex tracksMutex;  // Allow concurrent reads
    
public:
    // Read operation - shared lock
    int getLoadedTrackCount() const {
        std::shared_lock lock(tracksMutex);
        return tracks.size();
    }
    
    // Write operation - exclusive lock
    void addTrack(const std::string& path) {
        std::unique_lock lock(tracksMutex);
        tracks.push_back(loadTrack(path));
    }
};

---

8. Testing Coverage

Current State:

Only one test file: tests/GravityTests.cpp

Recommendation: Add comprehensive tests:

tests/
├── GravityTests.cpp          ✅ Exists
├── GameLogicTests.cpp        ❌ Missing
├── ScoreManagerTests.cpp     ❌ Missing
├── StateTransitionTests.cpp  ❌ Missing
└── AudioSystemTests.cpp      ❌ Missing

Example Test Structure:

// tests/GameLogicTests.cpp
#include <catch2/catch_test_macros.hpp>
#include "gameplay/core/Game.h"

TEST_CASE("Game initialization", "[game]") {
    Game game(0);
    
    SECTION("Board starts empty") {
        const auto& board = game.boardRef();
        REQUIRE(std::all_of(board.begin(), board.end(), 
                           [](int cell) { return cell == 0; }));
    }
    
    SECTION("Score starts at zero") {
        REQUIRE(game.score() == 0);
        REQUIRE(game.lines() == 0);
    }
}

TEST_CASE("Piece rotation", "[game]") {
    Game game(0);
    
    SECTION("Clockwise rotation") {
        auto initialRot = game.current().rot;
        game.rotate(1);
        REQUIRE(game.current().rot == (initialRot + 1) % 4);
    }
}

TEST_CASE("Line clearing", "[game]") {
    Game game(0);
    
    SECTION("Single line clear awards correct score") {
        // Setup: Fill bottom row except one cell
        // ... test implementation
    }
}

---

9. Documentation

Current State:

• Good inline comments
• Config.h has excellent documentation
• Missing: API documentation, architecture overview

Recommendation: Add Doxygen-style comments:

/**
 * @class Game
 * @brief Core Tetris game logic engine
 * 
 * Manages the game board, piece spawning, collision detection,
 * line clearing, and scoring. This class is independent of
 * rendering and input handling.
 * 
 * @note Thread-safe for read operations, but write operations
 *       (move, rotate, etc.) should only be called from the
 *       main game thread.
 * 
 * Example usage:
 * @code
 * Game game(5);  // Start at level 5
 * game.tickGravity(16.67);  // Update for one frame
 * if (game.isGameOver()) {
 *     // Handle game over
 * }
 * @endcode
 */
class Game {
    // ...
};

Create docs/ARCHITECTURE.md:

# Architecture Overview

## State Machine
[Diagram of state transitions]

## Data Flow
[Diagram showing how data flows through the system]

## Threading Model
- Main thread: Rendering, input, game logic
- Background thread: Audio loading
- Audio callback thread: Audio mixing

---

10. Performance Considerations

Issue: Frequent String Allocations

Location: Various places using std::string for paths

Recommendation:

// Use string_view for read-only string parameters
#include <string_view>

SDL_Texture* loadTextureFromImage(SDL_Renderer* renderer, 
                                  std::string_view path,  // Changed
                                  int* outW = nullptr, 
                                  int* outH = nullptr);

// For compile-time strings, use constexpr
namespace AssetPaths {
    constexpr std::string_view LOGO = "assets/images/logo.bmp";
    constexpr std::string_view BACKGROUND = "assets/images/main_background.bmp";
}

Issue: Vector Reallocations

Location: fireworks vector in main.cpp

Recommendation:

// Reserve capacity upfront
fireworks.reserve(5);  // Max 5 fireworks at once

// Or use a fixed-size container
std::array<std::optional<TetrisFirework>, 5> fireworks;

---

🔧 Specific Recommendations by Priority

High Priority (Do These First)

1. Replace raw SDL pointers with smart pointers

   • Impact: Prevents memory leaks
   • Effort: Medium
   • Files: MenuState.h, main.cpp, all state files

2. Remove debug file I/O from production code

   • Impact: Performance, code cleanliness
   • Effort: Low
   • Files: MenuState.cpp, main.cpp

3. Add error handling to asset loading

   • Impact: Better debugging, crash prevention
   • Effort: Medium
   • Files: main.cpp, AssetManager.cpp

Medium Priority

4. Extract common patterns into helper classes

   • Impact: Code maintainability
   • Effort: Medium
   • Files: All state files

5. Move remaining magic numbers to Config.h

   • Impact: Maintainability
   • Effort: Low
   • Files: MenuState.cpp, UIRenderer.cpp

6. Add comprehensive unit tests

   • Impact: Code quality, regression prevention
   • Effort: High
   • Files: New test files

Low Priority (Nice to Have)

7. Add Doxygen documentation

   • Impact: Developer onboarding
   • Effort: Medium

8. Performance profiling and optimization

   • Impact: Depends on current performance
   • Effort: Medium

9. Consider using std::expected for error handling

   • Impact: Better error handling
   • Effort: High (requires C++23 or external library)

---

📝 Code Style Observations

Good Practices You're Already Following:

✅ Consistent naming conventions:

• Classes: PascalCase (e.g., MenuState, GameRenderer)
• Functions: camelCase (e.g., tickGravity, loadTexture)
• Constants: UPPER_SNAKE_CASE (e.g., LOGICAL_W, DAS_DELAY)
• Member variables: camelCase with m_ prefix in some places

✅ Header guards: Using #pragma once

✅ Forward declarations: Minimizing include dependencies

✅ RAII: Resources tied to object lifetime

Minor Style Inconsistencies:

❌ Inconsistent member variable naming:

// Some classes use m_ prefix
float m_masterVolume = 1.0f;

// Others don't
int selectedButton = 0;

Recommendation: Pick one style and stick to it. I suggest:

// Private members: m_ prefix
float m_masterVolume = 1.0f;
int m_selectedButton = 0;

// Public members: no prefix (rare in good design)

---

🎨 Architecture Suggestions

Consider Implementing:

1. Event System Instead of callbacks, use an event bus:

   // events/GameEvents.h
   struct LineClearedEvent {
       int linesCleared;
       int newScore;
   };
   
   struct LevelUpEvent {
       int newLevel;
   };
   
   // EventBus.h
   class EventBus {
   public:
       template<typename Event>
       void subscribe(std::function<void(const Event&)> handler);
   
       template<typename Event>
       void publish(const Event& event);
   };
   
   // Usage in Game.cpp
   eventBus.publish(LineClearedEvent{linesCleared, _score});
   
   // Usage in Audio system
   eventBus.subscribe<LineClearedEvent>([](const auto& e) {
       playLineClearSound(e.linesCleared);
   });
   

2. Component-Based UI Extract UI components:

   class Button {
   public:
       void render(SDL_Renderer* renderer);
       bool isHovered(int mouseX, int mouseY) const;
       void onClick(std::function<void()> callback);
   };
   
   class Panel {
       std::vector<std::unique_ptr<UIComponent>> children;
   };
   

3. Asset Manager Centralize asset loading:

   class AssetManager {
   public:
       SDL_TexturePtr getTexture(std::string_view name);
       FontAtlas* getFont(std::string_view name);
   
   private:
       std::unordered_map<std::string, SDL_TexturePtr> textures;
       std::unordered_map<std::string, std::unique_ptr<FontAtlas>> fonts;
   };
   

---

🔍 Security Considerations

1. File Path Validation

   // AssetPath::resolveImagePath should validate paths
   // to prevent directory traversal attacks
   std::string resolveImagePath(std::string_view path) {
       // Reject paths with ".."
       if (path.find("..") != std::string_view::npos) {
           SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, 
                       "Invalid path: %s", path.data());
           return "";
       }
       // ... rest of implementation
   }
   

2. Score File Tampering Consider adding checksums to score files:

   // Scores.cpp
   void ScoreManager::save() const {
       nlohmann::json j;
       j["scores"] = scores;
       j["checksum"] = computeChecksum(scores);
       // ... save to file
   }
   

---

📊 Metrics

Based on the codebase analysis:

Metric	Value	Rating
Code Organization	Excellent	⭐⭐⭐⭐⭐
Modern C++ Usage	Very Good	⭐⭐⭐⭐
Error Handling	Fair	⭐⭐⭐
Memory Safety	Good	⭐⭐⭐⭐
Test Coverage	Poor	⭐
Documentation	Good	⭐⭐⭐⭐
Performance	Good	⭐⭐⭐⭐
Maintainability	Very Good	⭐⭐⭐⭐

Overall Score: 4/5 ⭐⭐⭐⭐

---

🚀 Quick Wins (Easy Improvements)

1. Add .clang-format file for consistent formatting
2. Create CONTRIBUTING.md with coding guidelines
3. Add pre-commit hooks for formatting and linting
4. Set up GitHub Actions for CI/CD
5. Add README.md with build instructions and screenshots

---

📚 Recommended Resources

• Modern C++ Best Practices: https://isocpp.github.io/CppCoreGuidelines/
• SDL3 Migration Guide: https://wiki.libsdl.org/SDL3/README/migration
• Game Programming Patterns: https://gameprogrammingpatterns.com/
• C++ Testing with Catch2: https://github.com/catchorg/Catch2

---

✅ Conclusion

Your Tetris project demonstrates strong software engineering practices with a clean architecture, modern C++ usage, and good separation of concerns. The main areas for improvement are:

1. Enhanced error handling
2. Increased test coverage
3. Elimination of raw pointers
4. Removal of debug code from production

With these improvements, this codebase would be production-ready and serve as an excellent example of modern C++ game development.

Keep up the excellent work! 🎮