2
\$\begingroup\$

I've been coding a helper container template to contain tiles in 2D games. What I figured out I would like when writing code for my games is forgetting all resize stuff, allowing negative indexes (for proceduraly generated worlds, that I hope to achieve in the future) and returning a default value (and not some error) if the index is out of bounds.

The most important part of the code is the operator[] overload.

Here is what I wrote (it's quite a chunk !):

#ifndef AUTOARRAY_H
#define AUTOARRAY_H

#include <vector>
#ifdef DO_DEBUG
#include <iostream>
#endif

//#define AUTOARRAY_ALLOW_GET //allows to acces real stl container directly
//#define AUTOARRAY_ALLOW_RSIZE //allows to get the real, cached size of the container
//#define AUTOARRAY_2D //include auto2DArray typedef.
//#define DO_DEBUG //do debug lines, such as prints and logs

namespace yk
{
    template<typename T>
    class autoArray
    {
        private:
            typedef std::vector<T> array_type;
            typedef typename array_type::iterator iterator_t;
            typedef typename array_type::const_iterator citerator_t;

            array_type m_array, m_negArray;
            T m_defVal;
            size_t m_fakeSize;
            size_t m_fakeNegSize;

            //helper init function
            inline void _init( const T& defaultValue, const array_type& initial, const array_type& negInitial )
            {
                m_defVal = defaultValue;
                m_array = initial;
                m_fakeSize = initial.size();
                m_negArray = negInitial;
                m_fakeNegSize = negInitial.size();
            }

            //cause it didn't work w/o ?
            inline bool _greaterThanSize( const int& n ) const
            { return static_cast<size_t>(n) > realSize(); }

            inline bool _greaterThanNegSize( const int& n ) const
            { return static_cast<size_t>(n) > realNegSize(); }

            /*inline void _resize( const int& index, array_type& ar, size_t& arraySize )
            {
                //Not doing it yet 'cause the code isn't realy duplicate and I
                //can't figure out an intelligent way to do it.
            }*/

        public:
            //default initialiser
            inline autoArray()
            { _init( T(), {}, {} ); }

            //initialiser with a default value when resizing.
            inline autoArray( const T& defaultValue )
            { _init( defaultValue, {}, {} ); }

            //initialiser with an initial array
            inline autoArray( const array_type& initial )
            { _init( T(), initial, {} ); }

            //initialiser with an initial array and negArray
            inline autoArray( const array_type& initial, const array_type& negInitial )
            { _init( T(), initial, negInitial ); }

            //initialiser with an initial array and default resize value
            inline autoArray( const T& defaultValue, const array_type& initial )
            { _init( defaultValue, initial, {} ); }

            //initialiser with an initial array, negArray and default resize value
            inline autoArray( const T& defaultValue, const array_type& initial, const array_type& negInitial )
            { _init( defaultValue, initial, negInitial ); }

            //so you only get the size of the used array, not the cached one.
            inline size_t size() const
            { return m_fakeSize; }

            inline size_t negSize() const
            { return m_fakeNegSize; }

            //if index is too big, resize accordingly and return defVal
            inline T& operator[]( const int& index )
            {
#ifdef DO_DEBUG
                std::cout << "index: " << index << " ";
#endif
                if( index >= 0 )
                {
                    //if the index is positive (or eq. to 0)
                    //use the positive array
#ifdef DO_DEBUG
                    std::cout << "index is positive" << std::endl;
#endif
                    if( _greaterThanSize( index + 1 ) )
                    {
                        //resize way bigger, cause resizing is slow.
                        //this speeds things up a bit by being overeager.
#ifdef DO_DEBUG
                        std::cout << "resizing pos array" << std::endl;
#endif
                        m_array.resize( ( index + 1 ) * 2, m_defVal );
                    }

                    if( static_cast<size_t>(index) + 1 > m_fakeSize )
                    {
                        //set fake size to the biggest index accessed,
                        //so the user doesn't get all the defvals when he iterates through.
#ifdef DO_DEBUG
                        std::cout << "seting fake size to " << m_fakeSize << std::endl;
#endif
                        m_fakeSize = static_cast<size_t>(index) + 1;
                    }

                    return m_array[index];
                } else
                {
                    //if the index is positive (or eq. to 0)
                    //use the negative array

                    int realIndex = -index - 1;
                    //the index of the value in the array starts at 1,
                    //index 0 being stored in he positive array
#ifdef DO_DEBUG
                    std::cout << "index is negative" << std::endl;
#endif
                    if( _greaterThanNegSize( realIndex + 1 ) )
                    {
                        //resize way bigger, cause resizing is slow.
                        //this speeds things up a bit by being overeager.
#ifdef DO_DEBUG
                        std::cout << "resizing neg array to " << ( realIndex + 1 ) * 2 << std::endl;
#endif
                        m_negArray.resize( ( realIndex + 1 ) * 2, m_defVal );
                    }

                    if( static_cast<size_t>( realIndex + 1 ) > m_fakeNegSize )
                    {
                        //set fake size to the biggest index accessed,
                        //so the user doesn't get all the defvals when he iterates through.
#ifdef DO_DEBUG
                        std::cout << "seting fake size to " << static_cast<size_t>( realIndex ) + 1  << std::endl;
#endif
                        m_fakeNegSize = static_cast<size_t>( realIndex ) + 1;
                    }
#ifdef DO_DEBUG
                    std::cout << "returning val in neg array at index " << realIndex << std::endl;
                    std::cout << "real size of array: " << m_negArray.size() << std::endl;
#endif
                    return m_negArray[realIndex];
                }


            }

            //index operator for situations when the entity is const
            inline T operator[]( const int& index ) const
            {
                /*
                //for some reason doesn't work
                if( _greaterThanSize(index) or _greaterThanNegSize( -index - 1))
                {
#ifdef DO_DEBUG
                    std::cout << "returning defval (index too small or too big)" << std::endl;
#endif
                    return m_defVal;
                }*/

                if( index >= 0 )
                {
#ifdef DO_DEBUG
                    std::cout << "returning at index " << index << " in posarray" << std::endl;
#endif
                    return m_array[index];
                }

                else
                {
#ifdef DO_DEBUG
                    std::cout << "returning at index " << index << " in negarray" << std::endl;
                    std::cout << "(real index : " << -index - 1 << ")" << std::endl;
#endif
                    return m_negArray[-index - 1];
                }
            }

            //For ranged based loops - to be modified.
            //I'll have to define my own type of iterator...
            /*
            inline iterator_t begin()
            { return m_array.begin(); }

            inline citerator_t begin() const
            { return m_array.begin(); }

            inline iterator_t end()
            { return m_array.end() - (realSize() - size()); }

            inline citerator_t end() const
            { return m_array.end() - (realSize() - size()); }
            */

            //for cout
            friend inline std::ostream& operator<<( std::ostream& stream, const autoArray<T>& oarray )
            {
#ifdef DO_DEBUG
                std::cout << "printing from " << -static_cast<int>(oarray.negSize()) << " to " << oarray.size() << std::endl;
#endif
                for(int i = -static_cast<int>(oarray.negSize()); i < static_cast<int>(oarray.size()); i++)
                {
                    stream << i << " : " << oarray[i] << std::endl;
                }

                return stream;
            }

            virtual ~autoArray() { }

#ifdef AUTOARRAY_ALLOW_GET
            inline array_type& get()
            { return m_array; }

            inline array_type& getNeg()
            { return m_negArray; }
#endif //AUTOARRAY_ALLOW_GET

        //if client doesnt use it, declare it private
#ifdef AUTOARRAY_ALLOW_RSIZE
        public:
#else
        private:
#endif //AUTOARRAY_ALLOW_RSIZE
            inline size_t realSize() const
            { return m_array.size(); }

            inline size_t realNegSize() const
            { return m_negArray.size(); }

    }; //autoArray class definition

#ifdef AUTOARRAY_2D
    template<typename T>
    using auto2DArray = autoArray<autoArray<T> >;

    template<typename T>
    inline auto2DArray<T> makeAuto2DwithDefault(const T& defVal)
    {
        autoArray<T> a( defVal );
        return auto2DArray<T>( a );
    }
#endif

} //namespace yk

#endif // AUTOARRAY_H

Couple of things I'm not sure about :

  1. Is the DO_DEBUG macro a good idea ? If not, what alternative should I use?
  2. I used 2 containers, one for the positive and 0 indexes, an other for the negative ones, because shifting all values seemed slow and tedious to me. Does this use significantly more memory?
  3. The basic aim of this class is to create 2D arrays, but I thought that making a generic version first was a better idea, leading me to the final typedef. Is including a typedef in function of a macro good design? It seems not enough to put in a file.
  4. How could I make this faster? It will possibly contain huge amounts of data in some occasions.
\$\endgroup\$
  • \$\begingroup\$ Is your compiler C++11 or C++14 compliant? \$\endgroup\$ – user2296177 May 12 '16 at 20:45
  • \$\begingroup\$ I think it's c++14 \$\endgroup\$ – Yk Cheese May 12 '16 at 21:00
  • \$\begingroup\$ I'm using GNU g++ \$\endgroup\$ – Yk Cheese May 12 '16 at 21:01
1
\$\begingroup\$

Use default values for your constructor parameters

You currently have 6 different constructors! We can reduce that number down to 2 constructors by using default parameter arguments. Here's what your constructors would look like:

// default ctor, every data member's default ctor is called
autoArray() = default;

// ctor to initialize members, note that initial and negInitial have default values
autoArray( const T& defaultValue, const container& initial = {}, const container& negInitial = {} )
    : m_defVal{ defaultValue } // we use the member initializer list
    , m_array{ initial }
    , m_negArray{ negInitial }
{
    // we no longer use _init()
}

You can now get rid of the _init() function and 4 other constructors. This is cleaner and much easier on the eyes than having to decipher which constructor is being called.

You might have noticed that the constructors no longer have the inline keyword and that the call to _init() is gone. That brings me to my next two points...

Do not use initialization functions

You can replace your initialization function with constructors that use the member initializer list. The advantage this offers is the fact that you will now be able to actually initialize your members directly.

Currently, your data members are default constructed, their arguments are copied onto the _init() function and then your data members are copy constructed inside the _init() function. Talk about wasteful!

This example demonstrates the issue:

#include <iostream>
#include <string>

struct Type
{
    Type() { std::cout << "default ctor\n"; }
    Type( int ) { std::cout << "int ctor\n"; }
};

struct TypeUser
{
    TypeUser() {}
    TypeUser( int value ) { init( value ); } // calls init function
    void init( int value ) { m_value = Type( value ); }
    Type m_value;
};

int main()
{
    TypeUser tu{ 0 };
}

If you run this program, you will see that "default ctor" is displayed before "int ctor". The above example would be fixed like so:

struct TypeUser
{
    TypeUser() {}
    TypeUser( int value )
        : m_value{ value } // we now use the member initializer list
    {}
    Type m_value;
};

If you run it again, you'll see that only "int ctor" is displayed. Thus, we avoid a bunch of unnecessary operations.

If you go back to my first point, you'll see that knowledge being applied.

Visit this page for more information on constructors:

http://en.cppreference.com/w/cpp/language/initializer_list

Remove the redundant 'inline' keyword

Functions that are declared and defined inside the class are implicitly inline. Using that keyword everywhere is thus redundant. Personally, it makes it harder on my eyes.

Naming

In C++, an array and a vector are different entities: we have std::array<> for non-growing static contiguous memory and std::vector<> as a the growing contiguous dynamic memory container.

Thus, naming a typedef of std::vector<> as array can be confusing. I suggest renaming that alias to vector_type or a more generic container.

typedef std::vector<T> container; // instead of typedef std::vector<T> array_type

Furthermore, the reason you confidently use array_type::iterator and array_type::const_iterator is that you know that any standard compliant container has these aliases. The same does not hold true for your type.

Consider renaming your typedefs to follow that same pattern so that other template code can make the same assumptions that you did.

Taking all of that into consideration, we end up with:

typedef std::vector<T> container; // a generic name in case you change the container type
typedef typename container::iterator iterator; // same name as found in standard containers
typedef typename container::const_iterator const_iterator; // as above

Anything else?

Why yes, there's actually a lot more to talk about, such as the questionable design choice of having a container that can be indexed into with negative values; that is definitely not commonplace and might surprise a few people.

Honestly, I'd recommend you take a step back and familiarize yourself with the features that C++ offers by getting a good book from this page:

https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot ! Anything to say about the macros ? \$\endgroup\$ – Yk Cheese May 13 '16 at 5:31
  • \$\begingroup\$ @YkCheese There are alternatives to using macros, such as policy based design for your class. You can look at Modern C++ Design by Andrei Alexandrescu for a book that goes over that. \$\endgroup\$ – user2296177 May 13 '16 at 15:18

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.