2
\$\begingroup\$

I've written some code for console variables. They support different types e.g. integer, string, vector, float, etc.

Is there a better way to do this? Like with an interface and a class for each different type? (I'm not sure how I would do that.) I also have to create a register function for each type of variable: float, int, etc.

Is there a way to do it with a template? E.g.

Console.RegisterConVar<float>("float_test", 1.2, 3.2);

Any other criticism is also welcome.

Vector.h:

#pragma once

class Vector2
{
public:
    Vector2() : fX(0.0f), fY(0.0f) {}
    Vector2(float x, float y) : fX(x), fY(y) {}
    float fX, fY;
};

class Vector3
{
public:
    Vector3() : fX(0.0f), fY(0.0f), fZ(0.0f) {}
    Vector3(float x, float y, float z) : fX(x), fY(y), fZ(z) {}
    float fX, fY, fZ;
};

class Vector4
{
public:
    Vector4() : fX(0.0f), fY(0.0f), fZ(0.0f), fA(0.0f) {}
    Vector4(float x, float y, float z, float a) : fX(x), fY(y), fZ(z), fA(a) {}
    float fX, fY, fZ, fA;
};

ConVar.h:

#pragma once

enum ConVarType
{
    CONVAR_TYPE_BOOL            = 0x0,
    CONVAR_TYPE_FLOAT           = 0x1,
    CONVAR_TYPE_VEC2            = 0x2,
    CONVAR_TYPE_VEC3            = 0x3,
    CONVAR_TYPE_VEC4            = 0x4,
    CONVAR_TYPE_INT             = 0x5,
    CONVAR_TYPE_ENUM            = 0x6,
    CONVAR_TYPE_STRING          = 0x7,
    CONVAR_TYPE_INT64           = 0x8
};

enum ConVarValueType
{
    CONVARVALUE_TYPE_CURRENT    = 0x0,
    CONVARVALUE_TYPE_DEFAULT    = 0x1
};

union ConVarLimits
{
    template<typename T>
    struct Limits
    {
        T min;
        T max;
    };

    Limits<int> integer;
    Limits<__int64> integer64;
    Limits<float> floating;
    std::vector<std::string>* enumList;
};

union ConVarValue
{
    ConVarValue() {}
    ~ConVarValue() {}
    bool boolean;
    int integer;
    __int64 integer64;
    float floating;
    Vector2 Vec2;
    Vector3 Vec3;
    Vector4 Vec4;
    const char* string;
};

class ConVar
{
public:
    std::string m_Name;
    ConVarType m_Type;
    ConVarValue m_Current;
    ConVarValue m_Default;
    ConVarLimits m_Domain;

    ~ConVar()
    {
        if(m_Type == CONVAR_TYPE_STRING)
            delete [] m_Current.string;

        if(m_Type == CONVAR_TYPE_ENUM)
        {
            delete m_Domain.enumList;
        }
    }

    bool GetValueString(ConVarValueType vType, char* buffer, size_t size)
    {
        ConVarValue* var = (vType == CONVARVALUE_TYPE_CURRENT) ? &this->m_Current : (vType == CONVARVALUE_TYPE_DEFAULT) ? &this->m_Default : nullptr;
        if(var == nullptr)
            return false;

        switch(this->m_Type)
        {
        case CONVAR_TYPE_BOOL:
            sprintf_s(buffer, size, "%s", (var->boolean) ? "1" : "0"); break;

        case CONVAR_TYPE_FLOAT:
            sprintf_s(buffer, size, "%g", var->floating); break;

        case CONVAR_TYPE_VEC2:
            sprintf_s(buffer, size, "%g %g", var->Vec2.fX, var->Vec2.fY); break;

        case CONVAR_TYPE_VEC3:
            sprintf_s(buffer, size, "%g %g %g", var->Vec3.fX, var->Vec3.fY, var->Vec3.fZ); break;

        case CONVAR_TYPE_VEC4:
            sprintf_s(buffer, size, "%g %g %g %g", var->Vec4.fX, var->Vec4.fY, var->Vec4.fZ, var->Vec4.fA); break;

        case CONVAR_TYPE_INT:
            sprintf_s(buffer, size, "%i", var->integer); break;

        case CONVAR_TYPE_INT64:
            sprintf_s(buffer, size, "%lli", var->integer64); break;

        case CONVAR_TYPE_ENUM:
            if(var->integer < 0 || var->integer >= static_cast<int>(m_Domain.enumList->size()))
            {
                sprintf_s(buffer, size, "");
                break;
            }
            else
            {
                const char* enumVal = m_Domain.enumList->at(var->integer).c_str();
                sprintf_s(buffer, size, "%s", (enumVal) ? enumVal : "" );
                break;
            }

        case CONVAR_TYPE_STRING:
            sprintf_s(buffer, size, "%s", (var->string) ? var->string : ""); break;

        default:
            sprintf_s(buffer, size, ""); break;
        }

        return true;
    }
};

IConsole.h:

#pragma once

class IConsole
{
public:
    ~IConsole()
    {
        for(auto& cvar : m_VarPool)
            delete cvar;
    }

    ConVar* RegisterNew(const std::string& name, ConVarType type, ConVarValue* value, ConVarLimits* domain)
    {
        if(FindConVar(name) == nullptr)
        {
            ConVar* Cvar = new ConVar();
            Cvar->m_Name = std::move(name);
            Cvar->m_Type = type;

            memcpy(&Cvar->m_Current, value, sizeof(ConVarValue));
            memcpy(&Cvar->m_Default, value, sizeof(ConVarValue));
            memcpy(&Cvar->m_Domain, domain, sizeof(ConVarLimits));

            m_VarPool.push_back(Cvar);
            m_VarMap[name] = Cvar;
            return Cvar;
        }
        return nullptr;
    }

    ConVar* RegisterInt(const std::string& name, int default_value, int min, int max)
    {
        ConVarValue value;
        ConVarLimits domain;

        value.integer = default_value;
        domain.integer.min = min;
        domain.integer.max = max;

        return RegisterNew(name, CONVAR_TYPE_INT, &value, &domain);
    }

    ConVar* RegisterFloat(const std::string& name, float default_value, float min, float max)
    {
        ConVarValue value;
        ConVarLimits domain;

        value.floating = default_value;
        domain.floating.min = min;
        domain.floating.max = max;

        return RegisterNew(name, CONVAR_TYPE_FLOAT, &value, &domain);
    }

    ConVar* RegisterString(const std::string& name, const char* string)
    {
        ConVarValue value;
        ConVarLimits domain;

        size_t len = strlen(string);
        char* newStr = new char[len+1];
        memcpy(newStr, string, len+1);
        newStr[len] = 0;

        value.string = newStr;
        return RegisterNew(name, CONVAR_TYPE_STRING, &value, &domain);
    }

    ConVar* RegisterEnum(const std::string& name, std::initializer_list<const char*> enumList, int defaultIndex)
    {
        ConVarValue value;
        ConVarLimits domain;

        value.integer = (defaultIndex > 0) ? defaultIndex : 0;
        domain.enumList = new std::vector<std::string>;

        for(auto& mEnum : enumList)
            domain.enumList->emplace_back(mEnum);

        return RegisterNew(name, CONVAR_TYPE_ENUM, &value, &domain);
    }

    ConVar* FindConVar(const std::string& name)
    {
        auto it = m_VarMap.find(name);
        return (it != m_VarMap.end()) ? (*it).second : nullptr;
    }

private:
    std::vector<ConVar*> m_VarPool;
    std::unordered_map<std::string, ConVar*> m_VarMap;
};

main.cpp:

#include <vector>
#include <unordered_map>
#include <iostream>
#include <string>

#include "Vector.h"
#include "ConVar.h"
#include "IConsole.h"

void main()
{
    IConsole Console;
    Console.RegisterInt("int_test", 76, 0, 100);
    Console.RegisterFloat("float_test", 33.0f, 0.0f, 100.0f);
    Console.RegisterEnum("enum_test", {"enum1", "enum2", "enum3", "enum4"}, 0);
    Console.RegisterString("string_test", "hello, this is a test string");

    while(true)
    {
        std::string name;
        std::cout << "Please enter cvar name: ";
        std::getline(std::cin, name);

        ConVar* Cvar = Console.FindConVar(name);
        if(Cvar != nullptr)
        {
            char buffer[256];
            Cvar->GetValueString(CONVARVALUE_TYPE_CURRENT, buffer, sizeof(buffer));
            std::cout << buffer << std::endl;
        }
        else
        {
            std::cout << "Invalid Cvar" << std::endl;
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

Having implemented similar code a couple times, I like your implementation. It is simple and gets the job done. You have chosen to go with a tagged union style, which has the upside that you don't have to rely on templates, so the class methods could be defined in a .cpp file instead, which would be recommended for a larger class. The main downside is relying on runtime type checking.

The other approach using templates would look something like this:

// An interface with the operations required, all methods are pure virtual.
class ICVar
{
public:
    virtual bool GetValueString(ConVarValueType vType, char* buffer, size_t size) = 0;
    virtual ~ICVar() = default;
    // etc...
};

// Then a small template implementation that can store any type you need.
template<typename DataType>
class CVarImpl
{
public:
    // TODO add the necessary constructors and destructor (if needed)

    bool GetValueString(ConVarValueType vType, char* buffer, size_t size) override
    {
        // Implement the necessary conversion logic using function overloads
        // for the supported types, e.g., something like 'std::to_string()'
    }

private:
    DataType currentValue;
    DataType defaultValue;
    // etc...
};

// Now this replaces your enum of types:
typedef CVarImpl<int>   CVarInt;
typedef CVarImpl<float> CVarFloat;
typedef CVarImpl<bool>  CVarBool;
// etc...

Then your Console Var manager gives the users pointers to ICVar, which are in turn an instance of a typed template instantiation (CVarImpl<int>, CVarImpl<float>, etc).

The benefit of this approach is getting rid of the runtime type switches. It also prevents some programming errors that stem from mismatching the type tag with the actual data type, but like I said in the beginning, your implementation is not bad. It is small and gets the job done, so this suggestion is just to give you a possible alternate solution you can compare yours with.

Now getting to the RegisterConVar<T> function is easy. All you have to do is define the templated method in the Console class:

// Pseudocode:
class Console
{
public:
    template<typename DataType>
    ICVar* RegisterConVar([arguments])
    {
        // perform the lookup, etc...
        ICVar* newVar = new CVarImpl<DataType>([arguments]);
    }
};

Other code improvement suggestions

  • Use the Standard sized types from <cstdint>. __int64 is a Microsoft extension that reduces the portability of your code. C++ now has a standard std::int64_t type.

  • sprintf_s is another non-portable Microsoft extension. I'm fairly sure that VS now properly supports std::snprintf, which is the C++11 Standard equivalent. Look it up and use it if your compiler supports it.

  • GetValueString would be better if it returned a std::string. Unless you really have a performance issue there, I'd go with the more modern string interface instead. Passing around char buffers with an accompanying size is very cumbersome and error prone. Optionally, then keep the current implementation but at least provide a wrapper that turns the char buffer into a std::string.

  • I would go for a unique_ptr there:

     ~IConsole()
     {
         for(auto& cvar : m_VarPool)
             delete cvar;
     }
    

    If you change m_VarPool to:

    std::vector<std::unique_ptr<ConVar>> m_VarPool;
    

    You can throw the destructor away since cleanup will be automated.

  • In the RegisterNew function, the use of std::move is incorrect:

    Cvar->m_Name = std::move(name);
    

    The name parameter is taken by const reference. You can't move from a const reference, so that's the same as no move at all (a copy). Either keep the const ref and strip that move or better, take by value and then move. Sink by value and move is the now recommended when storying parameters that way.

  • This block of code is reinventing the strdup function:

       size_t len = strlen(string);
       char* newStr = new char[len+1];
       memcpy(newStr, string, len+1);
       newStr[len] = 0;
    

    Unfortunately, strdup is not standard, so I cannot recommend using it. But do move that into a helper function at least. It should prove useful in other places. Or switch to a pure std::string interface as I suggested earlier so you won't have to manually copy strings.

  • Pro Tip: Make sure your header files are self contained. I see that all of your includes are in main.cpp. This is bad, if I forget to include <unordered_map> before IConsole.h it will break. I'm pretty sure users of your code don't care about the fact that you used unordered_map internally when they wish to use your code. Make sure each header includes all the other dependencies it needs.

  • IConsole is not what we normally call and interface class (it has no virtual methods), so I see no reason for the I prefix. This prefix is usually added to a name to indicate Interface.

\$\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.