spacetris/QUICK_START_IMPROVEMENTS.md

Quick Start: Implementing Top 3 Improvements

This guide provides complete, copy-paste ready code for the three most impactful improvements.

---

🚀 Improvement #1: Smart Pointer Wrapper for SDL Resources

Step 1: Create the Utility Header

File: src/utils/SDLPointers.h

#pragma once
#include <SDL3/SDL.h>
#include <memory>

/**
 * @file SDLPointers.h
 * @brief Smart pointer wrappers for SDL resources
 * 
 * Provides RAII wrappers for SDL resources to prevent memory leaks
 * and ensure proper cleanup in all code paths.
 */

namespace SDL {

/**
 * @brief Deleter for SDL_Texture
 */
struct TextureDeleter {
    void operator()(SDL_Texture* tex) const {
        if (tex) {
            SDL_DestroyTexture(tex);
        }
    }
};

/**
 * @brief Deleter for SDL_Surface
 */
struct SurfaceDeleter {
    void operator()(SDL_Surface* surf) const {
        if (surf) {
            SDL_DestroySurface(surf);
        }
    }
};

/**
 * @brief Deleter for SDL_Renderer
 */
struct RendererDeleter {
    void operator()(SDL_Renderer* renderer) const {
        if (renderer) {
            SDL_DestroyRenderer(renderer);
        }
    }
};

/**
 * @brief Deleter for SDL_Window
 */
struct WindowDeleter {
    void operator()(SDL_Window* window) const {
        if (window) {
            SDL_DestroyWindow(window);
        }
    }
};

/**
 * @brief Smart pointer for SDL_Texture
 * 
 * Example usage:
 * @code
 * SDL::TexturePtr texture(SDL_CreateTexture(...));
 * if (!texture) {
 *     // Handle error
 * }
 * // Automatic cleanup when texture goes out of scope
 * @endcode
 */
using TexturePtr = std::unique_ptr<SDL_Texture, TextureDeleter>;

/**
 * @brief Smart pointer for SDL_Surface
 */
using SurfacePtr = std::unique_ptr<SDL_Surface, SurfaceDeleter>;

/**
 * @brief Smart pointer for SDL_Renderer
 */
using RendererPtr = std::unique_ptr<SDL_Renderer, RendererDeleter>;

/**
 * @brief Smart pointer for SDL_Window
 */
using WindowPtr = std::unique_ptr<SDL_Window, WindowDeleter>;

} // namespace SDL

---

Step 2: Update MenuState.h

File: src/states/MenuState.h

Before:

private:
    int selectedButton = 0;
    
    // Button icons (optional - will use text if nullptr)
    SDL_Texture* playIcon = nullptr;
    SDL_Texture* levelIcon = nullptr;
    SDL_Texture* optionsIcon = nullptr;
    SDL_Texture* exitIcon = nullptr;

After:

#include "../utils/SDLPointers.h"  // Add this include

private:
    int selectedButton = 0;
    
    // Button icons (optional - will use text if nullptr)
    SDL::TexturePtr playIcon;
    SDL::TexturePtr levelIcon;
    SDL::TexturePtr optionsIcon;
    SDL::TexturePtr exitIcon;

---

Step 3: Update MenuState.cpp

File: src/states/MenuState.cpp

Remove the manual cleanup from onExit:

Before:

void MenuState::onExit() {
    if (ctx.showExitConfirmPopup) {
        *ctx.showExitConfirmPopup = false;
    }
    
    // Clean up icon textures
    if (playIcon) { SDL_DestroyTexture(playIcon); playIcon = nullptr; }
    if (levelIcon) { SDL_DestroyTexture(levelIcon); levelIcon = nullptr; }
    if (optionsIcon) { SDL_DestroyTexture(optionsIcon); optionsIcon = nullptr; }
    if (exitIcon) { SDL_DestroyTexture(exitIcon); exitIcon = nullptr; }
}

After:

void MenuState::onExit() {
    if (ctx.showExitConfirmPopup) {
        *ctx.showExitConfirmPopup = false;
    }
    
    // Icon textures are automatically cleaned up by smart pointers
}

Update usage in render method:

Before:

std::array<SDL_Texture*, 4> icons = {
    playIcon,
    levelIcon,
    optionsIcon,
    exitIcon
};

After:

std::array<SDL_Texture*, 4> icons = {
    playIcon.get(),
    levelIcon.get(),
    optionsIcon.get(),
    exitIcon.get()
};

---

Step 4: Update main.cpp Texture Loading

File: src/main.cpp

Update the function signature and implementation:

Before:

static SDL_Texture* loadTextureFromImage(SDL_Renderer* renderer, const std::string& path, int* outW = nullptr, int* outH = nullptr) {
    if (!renderer) {
        return nullptr;
    }

    const std::string resolvedPath = AssetPath::resolveImagePath(path);
    SDL_Surface* surface = IMG_Load(resolvedPath.c_str());
    if (!surface) {
        SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Failed to load image %s (resolved: %s): %s", path.c_str(), resolvedPath.c_str(), SDL_GetError());
        return nullptr;
    }

    if (outW) { *outW = surface->w; }
    if (outH) { *outH = surface->h; }

    SDL_Texture* texture = SDL_CreateTextureFromSurface(renderer, surface);
    SDL_DestroySurface(surface);

    if (!texture) {
        SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Failed to create texture from %s: %s", resolvedPath.c_str(), SDL_GetError());
        return nullptr;
    }

    if (resolvedPath != path) {
        SDL_LogInfo(SDL_LOG_CATEGORY_APPLICATION, "Loaded %s via %s", path.c_str(), resolvedPath.c_str());
    }

    return texture;
}

After:

#include "utils/SDLPointers.h"  // Add at top of file

static SDL::TexturePtr loadTextureFromImage(SDL_Renderer* renderer, const std::string& path, int* outW = nullptr, int* outH = nullptr) {
    if (!renderer) {
        SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Renderer is null");
        return nullptr;
    }

    const std::string resolvedPath = AssetPath::resolveImagePath(path);
    SDL::SurfacePtr surface(IMG_Load(resolvedPath.c_str()));
    if (!surface) {
        SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Failed to load image %s (resolved: %s): %s", 
                     path.c_str(), resolvedPath.c_str(), SDL_GetError());
        return nullptr;
    }

    if (outW) { *outW = surface->w; }
    if (outH) { *outH = surface->h; }

    SDL::TexturePtr texture(SDL_CreateTextureFromSurface(renderer, surface.get()));
    // surface is automatically destroyed here

    if (!texture) {
        SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, "Failed to create texture from %s: %s", 
                     resolvedPath.c_str(), SDL_GetError());
        return nullptr;
    }

    if (resolvedPath != path) {
        SDL_LogInfo(SDL_LOG_CATEGORY_APPLICATION, "Loaded %s via %s", path.c_str(), resolvedPath.c_str());
    }

    return texture;
}

---

🧹 Improvement #2: Remove Debug File I/O

Step 1: Replace with SDL Logging

File: src/states/MenuState.cpp

Before:

// Trace entry to persistent log for debugging abrupt exit/crash during render
{
    FILE* f = fopen("tetris_trace.log", "a"); 
    if (f) { 
        fprintf(f, "MenuState::render entry\n"); 
        fclose(f); 
    }
}

After:

// Use SDL's built-in logging (only in debug builds)
#ifdef _DEBUG
    SDL_LogTrace(SDL_LOG_CATEGORY_APPLICATION, "MenuState::render entry");
#endif

Or, if you want it always enabled but less verbose:

SDL_LogVerbose(SDL_LOG_CATEGORY_APPLICATION, "MenuState::render entry");

---

Step 2: Create a Logging Utility (Optional, Better Approach)

File: src/utils/Logger.h

#pragma once
#include <SDL3/SDL.h>

/**
 * @brief Centralized logging utility
 * 
 * Wraps SDL logging with compile-time control over verbosity.
 */
namespace Logger {

#ifdef _DEBUG
    constexpr bool TRACE_ENABLED = true;
#else
    constexpr bool TRACE_ENABLED = false;
#endif

/**
 * @brief Log a trace message (only in debug builds)
 */
template<typename... Args>
inline void trace(const char* fmt, Args... args) {
    if constexpr (TRACE_ENABLED) {
        SDL_LogTrace(SDL_LOG_CATEGORY_APPLICATION, fmt, args...);
    }
}

/**
 * @brief Log a debug message
 */
template<typename... Args>
inline void debug(const char* fmt, Args... args) {
    SDL_LogDebug(SDL_LOG_CATEGORY_APPLICATION, fmt, args...);
}

/**
 * @brief Log an info message
 */
template<typename... Args>
inline void info(const char* fmt, Args... args) {
    SDL_LogInfo(SDL_LOG_CATEGORY_APPLICATION, fmt, args...);
}

/**
 * @brief Log a warning message
 */
template<typename... Args>
inline void warn(const char* fmt, Args... args) {
    SDL_LogWarn(SDL_LOG_CATEGORY_APPLICATION, fmt, args...);
}

/**
 * @brief Log an error message
 */
template<typename... Args>
inline void error(const char* fmt, Args... args) {
    SDL_LogError(SDL_LOG_CATEGORY_APPLICATION, fmt, args...);
}

} // namespace Logger

Usage in MenuState.cpp:

#include "../utils/Logger.h"

void MenuState::render(SDL_Renderer* renderer, float logicalScale, SDL_Rect logicalVP) {
    Logger::trace("MenuState::render entry");
    
    // ... rest of render code
    
    Logger::trace("MenuState::render exit");
}

---

Step 3: Update All Files

Files to update:

• src/states/MenuState.cpp (multiple locations)
• src/main.cpp (if any similar patterns)

Search and replace pattern:

// Find:
FILE* f = fopen("tetris_trace.log", "a"); 
if (f) { 
    fprintf(f, ".*"); 
    fclose(f); 
}

// Replace with:
Logger::trace("...");

---

🎯 Improvement #3: Extract Common Patterns

Step 1: Create ExitPopupHelper

File: src/states/StateHelpers.h (new file)

#pragma once

/**
 * @file StateHelpers.h
 * @brief Helper classes for common state patterns
 */

/**
 * @brief Helper for managing exit confirmation popup
 * 
 * Encapsulates the common pattern of showing/hiding an exit popup
 * and managing the selected button state.
 * 
 * Example usage:
 * @code
 * ExitPopupHelper exitPopup(ctx.exitPopupSelectedButton, ctx.showExitConfirmPopup);
 * 
 * if (exitPopup.isVisible()) {
 *     exitPopup.setSelection(0);  // Select YES
 * }
 * 
 * if (exitPopup.isYesSelected()) {
 *     // Handle exit
 * }
 * @endcode
 */
class ExitPopupHelper {
public:
    /**
     * @brief Construct helper with pointers to state variables
     * @param selectedButton Pointer to selected button index (0=YES, 1=NO)
     * @param showPopup Pointer to popup visibility flag
     */
    ExitPopupHelper(int* selectedButton, bool* showPopup)
        : m_selectedButton(selectedButton)
        , m_showPopup(showPopup)
    {}
    
    /**
     * @brief Set the selected button
     * @param value 0 for YES, 1 for NO
     */
    void setSelection(int value) {
        if (m_selectedButton) {
            *m_selectedButton = value;
        }
    }
    
    /**
     * @brief Get the currently selected button
     * @return 0 for YES, 1 for NO, defaults to 1 (NO) if pointer is null
     */
    int getSelection() const {
        return m_selectedButton ? *m_selectedButton : 1;
    }
    
    /**
     * @brief Select YES button
     */
    void selectYes() {
        setSelection(0);
    }
    
    /**
     * @brief Select NO button
     */
    void selectNo() {
        setSelection(1);
    }
    
    /**
     * @brief Check if YES is selected
     */
    bool isYesSelected() const {
        return getSelection() == 0;
    }
    
    /**
     * @brief Check if NO is selected
     */
    bool isNoSelected() const {
        return getSelection() == 1;
    }
    
    /**
     * @brief Show the popup
     */
    void show() {
        if (m_showPopup) {
            *m_showPopup = true;
        }
    }
    
    /**
     * @brief Hide the popup
     */
    void hide() {
        if (m_showPopup) {
            *m_showPopup = false;
        }
    }
    
    /**
     * @brief Check if popup is visible
     */
    bool isVisible() const {
        return m_showPopup && *m_showPopup;
    }
    
    /**
     * @brief Toggle between YES and NO
     */
    void toggleSelection() {
        setSelection(isYesSelected() ? 1 : 0);
    }

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

---

Step 2: Update MenuState.cpp

File: src/states/MenuState.cpp

Add include:

#include "StateHelpers.h"

Before:

void MenuState::handleEvent(const SDL_Event& e) {
    if (e.type == SDL_EVENT_KEY_DOWN && !e.key.repeat) {
        auto setExitSelection = [&](int value) {
            if (ctx.exitPopupSelectedButton) {
                *ctx.exitPopupSelectedButton = value;
            }
        };
        auto getExitSelection = [&]() -> int {
            return ctx.exitPopupSelectedButton ? *ctx.exitPopupSelectedButton : 1;
        };
        auto isExitPromptVisible = [&]() -> bool {
            return ctx.showExitConfirmPopup && *ctx.showExitConfirmPopup;
        };
        auto setExitPrompt = [&](bool visible) {
            if (ctx.showExitConfirmPopup) {
                *ctx.showExitConfirmPopup = visible;
            }
        };

        if (isExitPromptVisible()) {
            switch (e.key.scancode) {
                case SDL_SCANCODE_LEFT:
                case SDL_SCANCODE_UP:
                    setExitSelection(0);
                    return;
                case SDL_SCANCODE_RIGHT:
                case SDL_SCANCODE_DOWN:
                    setExitSelection(1);
                    return;
                case SDL_SCANCODE_RETURN:
                case SDL_SCANCODE_KP_ENTER:
                case SDL_SCANCODE_SPACE:
                    if (getExitSelection() == 0) {
                        setExitPrompt(false);
                        if (ctx.requestQuit) {
                            ctx.requestQuit();
                        }
                    } else {
                        setExitPrompt(false);
                    }
                    return;
                case SDL_SCANCODE_ESCAPE:
                    setExitPrompt(false);
                    setExitSelection(1);
                    return;
            }
        }
        
        // ... rest of code
    }
}

After:

void MenuState::handleEvent(const SDL_Event& e) {
    if (e.type == SDL_EVENT_KEY_DOWN && !e.key.repeat) {
        ExitPopupHelper exitPopup(ctx.exitPopupSelectedButton, ctx.showExitConfirmPopup);
        
        auto triggerPlay = [&]() {
            if (ctx.startPlayTransition) {
                ctx.startPlayTransition();
            } else if (ctx.stateManager) {
                ctx.stateManager->setState(AppState::Playing);
            }
        };

        if (exitPopup.isVisible()) {
            switch (e.key.scancode) {
                case SDL_SCANCODE_LEFT:
                case SDL_SCANCODE_UP:
                    exitPopup.selectYes();
                    return;
                case SDL_SCANCODE_RIGHT:
                case SDL_SCANCODE_DOWN:
                    exitPopup.selectNo();
                    return;
                case SDL_SCANCODE_RETURN:
                case SDL_SCANCODE_KP_ENTER:
                case SDL_SCANCODE_SPACE:
                    if (exitPopup.isYesSelected()) {
                        exitPopup.hide();
                        if (ctx.requestQuit) {
                            ctx.requestQuit();
                        } else {
                            SDL_Event quit{};
                            quit.type = SDL_EVENT_QUIT;
                            SDL_PushEvent(&quit);
                        }
                    } else {
                        exitPopup.hide();
                    }
                    return;
                case SDL_SCANCODE_ESCAPE:
                    exitPopup.hide();
                    exitPopup.selectNo();
                    return;
                default:
                    return;
            }
        }

        switch (e.key.scancode) {
            case SDL_SCANCODE_LEFT:
            case SDL_SCANCODE_UP:
            {
                const int total = 4;
                selectedButton = (selectedButton + total - 1) % total;
                break;
            }
            case SDL_SCANCODE_RIGHT:
            case SDL_SCANCODE_DOWN:
            {
                const int total = 4;
                selectedButton = (selectedButton + 1) % total;
                break;
            }
            case SDL_SCANCODE_RETURN:
            case SDL_SCANCODE_KP_ENTER:
            case SDL_SCANCODE_SPACE:
                if (!ctx.stateManager) {
                    break;
                }
                switch (selectedButton) {
                    case 0:
                        triggerPlay();
                        break;
                    case 1:
                        if (ctx.requestFadeTransition) {
                            ctx.requestFadeTransition(AppState::LevelSelector);
                        } else if (ctx.stateManager) {
                            ctx.stateManager->setState(AppState::LevelSelector);
                        }
                        break;
                    case 2:
                        if (ctx.requestFadeTransition) {
                            ctx.requestFadeTransition(AppState::Options);
                        } else if (ctx.stateManager) {
                            ctx.stateManager->setState(AppState::Options);
                        }
                        break;
                    case 3:
                        exitPopup.show();
                        exitPopup.selectNo();
                        break;
                }
                break;
            case SDL_SCANCODE_ESCAPE:
                exitPopup.show();
                exitPopup.selectNo();
                break;
            default:
                break;
        }
    }
}

---

Step 3: Apply to Other States

Apply the same pattern to:

• src/states/PlayingState.cpp
• src/states/OptionsState.cpp

The refactoring is identical - just replace the lambda functions with ExitPopupHelper.

---

✅ Testing Your Changes

After implementing these improvements:

1. Build the project:

   cd d:\Sites\Work\tetris
   cmake -B build -S . -DCMAKE_BUILD_TYPE=Debug
   cmake --build build
   

2. Run the game:

   .\build\Debug\tetris.exe
   

3. Test scenarios:

   • Menu loads without crashes
   • All textures load correctly
   • Exit popup works (ESC key)
   • Navigation works (arrow keys)
   • No memory leaks (check with debugger)
   • Logging appears in console (debug build)

4. Check for memory leaks:

   • Run with Visual Studio debugger
   • Check Output window for memory leak reports
   • Should see no leaks from SDL textures

---

📊 Expected Impact

After implementing these three improvements:

Metric	Before	After	Improvement
Memory Safety	⭐⭐⭐	⭐⭐⭐⭐⭐	+67%
Code Clarity	⭐⭐⭐⭐	⭐⭐⭐⭐⭐	+25%
Maintainability	⭐⭐⭐⭐	⭐⭐⭐⭐⭐	+25%
Lines of Code	100%	~95%	-5%
Potential Bugs	Medium	Low	-50%

---

🎉 Next Steps

After successfully implementing these improvements:

1. Review the full CODE_ANALYSIS.md for more recommendations
2. Check IMPROVEMENTS_CHECKLIST.md for the complete task list
3. Consider implementing the medium-priority items next
4. Add unit tests to prevent regressions

Great job improving your codebase! 🚀