4
\$\begingroup\$

So I'm currently working on sort of an SDL engine (may be too ambitious I know). I'm seeking that it would be my first actual project and it's also the first time I'm seriously using classes. So I would like to know if there are any significant improvements I can make to this class to make it as good as it can be (having a class that randomly fails is kind of suicide). The one thing that bugs me at the moment is that the copy constructor and the copy assignment operator is exactly the same. If that's not fixable, are there any underlying problems I could fix? Thanks in advance.

#ifndef TEXT_H
#define TEXT_H

#define TEXTSIZE 50
#define FONTPATHSIZE 50
#define DEFAULTFONTSIZE 24
#define DEFAULTFONTCOLOR {255, 255, 255}
#define DEFAULTFONT "Fonts/Ubuntu.ttf"

class Text
{
    public:
        Text(){
            loaderFunction = TTF_RenderText_Blended;

            good      = false;
            visable   = true;
            numerical = false;

            renderer = NULL;
            sprite   = NULL;
            number   = NULL;
            font     = NULL;

            oldNumber = 0;

            fontSize  = DEFAULTFONTSIZE;
            textColor = DEFAULTFONTCOLOR;

            strcpy(this->fontPath, DEFAULTFONT);
            strcpy(this->text, "");

            ZeroMemory(&srcRect, sizeof(srcRect));
            ZeroMemory(&dstRect, sizeof(srcRect));         
        }
        Text(const Text& passedObject){
            strcpy(this->text, passedObject.text);
            strcpy(this->fontPath, passedObject.fontPath);

            this->loaderFunction = passedObject.loaderFunction;
            this->numerical      = passedObject.numerical;
            this->textColor      = passedObject.textColor;
            this->oldNumber      = passedObject.oldNumber;
            this->fontSize       = passedObject.fontSize;   
            this->renderer       = passedObject.renderer;
            this->srcRect        = passedObject.srcRect;
            this->dstRect        = passedObject.dstRect;
            this->visable        = passedObject.visable;
            this->number         = passedObject.number;
            this->good           = passedObject.good;
            this->font           = TTF_OpenFont(this->fontPath, this->fontSize);
            Load();              // Sets the sprite
        }
        ~Text(){
            SDL_DestroyTexture(this->sprite);
            TTF_CloseFont(this->font);
        }
        Text& operator=(const Text& passedObject){
            strcpy(this->text, passedObject.text);
            strcpy(this->fontPath, passedObject.fontPath);

            this->loaderFunction = passedObject.loaderFunction;
            this->numerical      = passedObject.numerical;
            this->textColor      = passedObject.textColor;
            this->oldNumber      = passedObject.oldNumber;
            this->fontSize       = passedObject.fontSize;
            this->renderer       = passedObject.renderer;
            this->srcRect        = passedObject.srcRect;
            this->dstRect        = passedObject.dstRect;
            this->visable        = passedObject.visable;
            this->number         = passedObject.number;
            this->good           = passedObject.good;
            this->font           = TTF_OpenFont(this->fontPath, this->fontSize);
            Load();              // Sets the sprite

            return *this;
        }
        Text& operator=(const char* text){
            ChangeText(text);

            return *this;
        }

        // Functionality functions
        void Initialize(SDL_Renderer* renderer, const char* text, const char* fontPath, int fontSize, SDL_Color textColor, int x, int y, bool numerical, int* number = NULL){
            strcpy(this->fontPath, fontPath);
            strcpy(this->text, text);

            this->renderer  = renderer;
            this->fontSize  = fontSize;
            this->textColor = textColor;
            this->numerical = numerical;
            this->number    = number;
            this->dstRect.x = x;
            this->dstRect.y = y;
            this->font      = TTF_OpenFont(this->fontPath, this->fontSize);

            this->good      = true;
            Load();
        }
        void Initialize(SDL_Renderer* renderer, const char* text, int x, int y){
            strcpy(this->text, text);

            this->renderer  = renderer;
            this->dstRect.x = x;
            this->dstRect.y = y;
            this->font      = TTF_OpenFont(this->fontPath, this->fontSize);

            this->good      = true;
            Load();
        }
        void Draw(){
            if(visable && good) SDL_RenderCopy(renderer, sprite, &srcRect, &dstRect);
        }
        void Load(){
            if(sprite     != NULL) SDL_DestroyTexture(sprite);
            if(this->font == NULL) return;

            SDL_Surface* loader = loaderFunction(this->font, this->text, this->textColor);
            sprite              = SDL_CreateTextureFromSurface(this->renderer, loader);

            srcRect.w = loader->w;
            srcRect.h = loader->h;
            dstRect.w = loader->w;
            dstRect.h = loader->h;

            SDL_FreeSurface(loader);
        }
        void Update(){
            if(!numerical || oldNumber == *number) return;
            if(sprite != NULL) SDL_DestroyTexture(sprite);

            // Create a temporary string
            static char temp[TEXTSIZE];
            strcpy(temp, this->text);
            sprintf(temp, "%s%d", temp, *number);

            SDL_Surface* loader = loaderFunction(this->font, temp, this->textColor);
            sprite              = SDL_CreateTextureFromSurface(this->renderer, loader);

            //Resize the text
            srcRect.w = loader->w;
            dstRect.w = loader->w;
            srcRect.h = loader->h;
            dstRect.h = loader->h;

            oldNumber = *number;
            SDL_FreeSurface(loader);
        }

        // Manipulation functions       
        void ChangeText(const char* text){
            if(!strcmp(this->text, text)) return;
            strcpy(this->text, text);

            // Reload the sprite
            Load();
        }      
        void ChangeTextColor(SDL_Color textColor){
            this->textColor = textColor;

            // Reload the sprite
            Load();
        }
        void ChangeFont(const char* fontPath){
            if(!strcmp(this->fontPath, fontPath)) return;
            strcpy(this->fontPath, fontPath);

            if(this->font != NULL) TTF_CloseFont(font);

            // Load the new font
            font = TTF_OpenFont(this->fontPath, this->fontSize);

            // Reload the sprite
            Load();
        }
        void ChangeFontSize(int fontSize){
            if(this->fontSize == fontSize) return;
            if(this->font != NULL) TTF_CloseFont(font);

            this->fontSize = fontSize;

            // Load the new font
            font = TTF_OpenFont(this->fontPath, this->fontSize);            

            // Reload the sprite
            Load();
        }
        void ChangeRenderer(SDL_Renderer* renderer){
            this->renderer = renderer;
        }
        void ChangeVisability(bool visable){
            this->visable = visable;
        }
        void ChangeLoaderFunction(SDL_Surface* (*loaderFunction)(TTF_Font*, const char*, SDL_Color)){
            this->loaderFunction = loaderFunction;
        }
        void ChangeNumber(int* number){
            this->number = number;
        }
        void SetNumericality(bool numerical){
            this->numerical = numerical;
        }
        void SetX(int x){
            this->dstRect.x = x;
        }
        void SetY(int y){
            this->dstRect.y = y;
        }

        // Variables
        SDL_Surface* (*loaderFunction)(TTF_Font*, const char*, SDL_Color);

        char text[TEXTSIZE], fontPath[FONTPATHSIZE];
        bool good, visable, numerical;
        int fontSize, *number, oldNumber;

        SDL_Color textColor;
        TTF_Font* font;

        SDL_Rect srcRect, dstRect;
        SDL_Renderer* renderer;
        SDL_Texture* sprite; 
};

#endif
\$\endgroup\$
4
\$\begingroup\$

Code:

  • Prefix the header guard with something unique (at least to the project) to prevent possible collisions. (e.g. MYPROJNAME_TEXT_H ).

    #ifndef PROJECTNAME_TEXT_H
    #define PROJECTNAME_TEXT_H
    
    ...
    
    #endif // PROJECTNAME_TEXT_H
    

    or use #pragma once instead:

    #pragma once
    
    ...
    
  • Wrapping everything in the project in a project namespace is also sensible.

  • Header files should be self-sufficient in terms of includes. The user (you!) should be able to include only the header file #include <Text.h>, but at the moment this won't compile, as the header files for various things used in Text.h would have to be included manually:

    #include <SDL.h>
    #include <SDL_TTF.h>
    #include <Windows.h>
    #include <string.h>
    ...
    
    #include <Text.h>
    
    ...
    
        Text text;
        text.Initialize(...); 
    ...
    

    which gets tiresome quickly. It's better to include them in one place (the header that needs them).

  • Function declarations should be in the header file, but function definitions should be in an accompanying .cpp file. This makes the class interface more readable, and more importantly reduces the amount of things we have to include in the header.


  • Use std::string instead of fixed-size character arrays because:

    • It's variable length (and it's quite reasonable for the text to be >50 characters, or for a font file path to be >50 characters).
    • Copy or comparison knows the length without having to parse the whole thing to find the null character at the end.
    • The std::string interface is much easier to use!
  • Use nullptr not NULL for null pointer values.

  • Use constant variables, not macros (macros are not scoped to your project and may affect other code). In this case the variables can be static const inside the Text class:

    class Text
    {
    public:
    
        static const std::string DefaultFont;
        static const int DefaultFontSize = 24;
        static const SDL_Color DefaultFontColor;
    
        ...
    };
    
    ... in Text.cpp:
    
    const std::string Text::DefaultFont = "Fonts/Ubuntu.ttf";
    const SDL_Color Text::DefaultFontColor = SDL_Color{ 255, 255, 255, 255 }; // it has 4 members!
    
  • Use a constructor initializer list to initialize variables, and make sure to initialize them in the same order as they are initialized in the class. (And don't use ZeroMemory; initialize the actual struct members):

    ... in Text.cpp:
    
    Text::Text():
        m_loaderFunction(TTF_RenderText_Blended),
        m_text(),
        m_fontPath(DefaultFont),
        ...
        srcRect{ 0, 0, 0, 0 },
        ...
        m_sprite(nullptr) {
    
        ...
    }
    
  • Make the member variables private! This ensures that the correct interface is used to change the class state.

  • Using this-> explicitly for every member variable access is quite wordy (and mainly unnecessary). Adding a prefix like m_ to member variable names (e.g. m_fontSize) is probably neater.


  • It's fine for the copy and assignment to look much the same. At the moment though, it looks like we can leak font objects. Load() checks for non-null sprites in the current object and destroys them; we need something similar for the font.

  • Making the class copyable could be a bad idea. If two textures are exactly the same, we generally only want one instance of the texture in memory. Perhaps a Text Clone() function would be a reasonable choice.


Design:

  • Storing a number inside the class is quite confusing. Especially as we use a pointer to some external value. This also is potentially dangerous if the pointed to variable is destroyed before the Text instance. It's much cleaner to simply update the text text.ChangeText(std::to_string(n)); and remove all the numeric stuff from the class.

  • This brings up another point. We don't know what this class represents externally (a number, an enum, actual text). If the external code has to check whether a number has changed to see if the text needs updating, it might as well do the same for actual text. In that case, there's no point in storing the text in the Text class either!

  • We know if the class is good if the texture is not null. So we don't need a separate variable for this, and could just add a bool IsGood() const;.

  • The class is storing both an SDL_TTF font, and an SDL_Texture containing a rendered string. However, it's extremely likely that multiple strings are going to be rendered with the same font, so storing a separate font object with every text string is a waste. We can split the class up into a separate Font and Text class.

  • Note that at this point, the Text class is almost a generic Sprite / Quad class, since it only contains the SDL_Texture, and the things needed to render it.

An interface more like the following might be better:

class Font
{
public:

    Font();
    Font(std::string const& fontPath, int fontSize);

    ~Font(); // calls TTF_CloseFont();

    // not copyable
    Font(Font const&) = delete;
    Font& operator=(Font const&) = delete;

    // moveable -> copy the pointer over, then set it to null in the original (i.e. transfer ownership)
    Font(Font&& other);
    Font& operator=(Font&& other);

    SDL_Surface* RenderText(std::string const& text, SDL_Color const& color);

    bool IsLoaded() const;

private:

    TTF_Font* m_font;
};

class Sprite
{
public:

    Sprite();
    Sprite(SDL_Renderer* renderer, SDL_Surface* surface); // creates surface and src rect.

    ~Sprite(); // destroys texture

    // not copyable
    Sprite(Sprite const&) = delete;
    Sprite& operator=(Sprite const&) = delete;

    // moveable -> copy the pointer and other data. set the pointer to null in the original (i.e. transfer ownership)
    Sprite(Sprite&& other);
    Sprite& operator=(Sprite&& other);

    void Render(SDL_Renderer* renderer, int x, int y);

    bool IsGood() const; // is m_sprite not a nullptr
    bool IsVisible() const;

private:

    bool m_visible;
    SDL_Texture* m_sprite;
    SDL_Rect m_src;
};

...

    Font font("Fonts/Ubuntu.ttf", 24);

    auto surface1 = font.RenderText("hello world!", SDL_Color{ 255, 255, 255, 255 });
    Sprite sprite1(renderer, surface1);

    auto surface1 = font.RenderText("asdlkfh!", SDL_Color{ 255, 0, 0, 255 });
    Sprite sprite2(renderer, surface2);

    sprite1.Render(renderer, 0, 0);
    sprite2.Render(renderer, 50, 50);

...
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.