2
\$\begingroup\$

I know I should be using vectors, but I want to learn something.

Please review it and point out any mistakes. Thank you. Here it is:

template< typename T >
class Table2D {
public:
    Table2D( unsigned int sizeX,
             unsigned int sizeY )
            : m_sizeX( sizeX )
            , m_sizeY( sizeY )
    {
        try {
            m_2DTable = new T[ m_sizeX * m_sizeY ]( );
        }
        catch( std::bad_alloc ) {
            std::cerr << "bad_alloc...\n";
        }
    }
    ~Table2D( ) { delete [ ] m_2DTable; }
private:
    unsigned int m_sizeX;
    unsigned int m_sizeY;
    T* m_2DTable;
public:
    void set( unsigned int x, unsigned int y, T val ) {
        m_2DTable[ x + ( y * m_sizeX ) ] = val;
    };
    T get( unsigned int x, unsigned int y ) {
        return m_2DTable[ x + ( y * m_sizeX ) ];
    };
};

Usage example:

Table2D< unsigned int > ui_2DTable( 50, 50 );
ui_2DTable.set( 10, 20, 50 );
std::cout << ui_2DTable.get( 10, 20 ) << std::endl;

Table2D< float > f_2DTable( 3, 7 );
f_2DTable.set( 1, 4, 1.75 );
std::cout << f_2DTable.get( 1, 4 ) << std::endl;
\$\endgroup\$
2
\$\begingroup\$

Firstly, I assume you have something like #define __in up the top. This is sort of a Microsoft-ism, and really isn't needed in most of these parameter sets. When you're passing types that are neither pointers nor references, it's pretty obvious that they're input values. To me, it's simply extra clutter which makes the code harder to read and provides no benefit.

Secondly, this seems like an extremely memory heavy way of doing what is basically a map. You allocate space for m_SizeX * m_SizeY elements, but if the table is sparse, a majority of that space is going to go unused.

Thirdly, you should be passing and returning certain values by (const) &. For example, in your set method should probably be:

void set(unsigned int x, unsigned int y, const T& val)

And your get method should be:

T& get(unsigned int x, unsigned int y)

This should also have a const overload:

const T& get(unsigned int x, unsigned int y) const

Note that if you haven't initialized a value, it will happily return you whatever junk happened to be there when new allocated the memory. This is probably not what you want - you should likely have it return either a default value or error.

If I were to write this, I'd utilize a std::map and something like the following:

#include <map>
#include <utility>

template <typename T>
class Table2D
{
private:
    std::map<std::pair<unsigned, unsigned>, T> table;

public:
    T& operator()(unsigned x, unsigned y)
    {
        std::pair<unsigned, unsigned> p = std::make_pair(x, y);
        return table[p];
    }

    const T& operator()(unsigned x, unsigned y) const
    {
        std::pair<unsigned, unsigned> p = std::make_pair(x, y);
        return table[p];
    }
};

This code assumes you want a default T() value (which will be 0 for all numeric types) to be inserted if it doesn't exist. Utilizing operator() returning a T& allows us to set and get values utilizing the same method:

Table2D<unsigned int> ui_2DTable;
ui_2DTable(10, 20) = 50;
std::cout << ui_2DTable(10, 20) << "\n";

Further, if we do something like:

std::cout << f_2DTable.get(10, 10) << "\n";

We'll simply return a default value. Currently, this will return something from a random bit of memory, and causes undefined behaviour, which is bad.

Finally, your code can cause undefined behaviour if the user tries to utilize the copy constructor or copy assignment operator. That is:

Table2D<unsigned int> ui_2DTable( 50, 50 );
ui_2DTable.set( 10, 20, 50 );
Table2D<unsigned int> ui_construct(ui_2DTable);
Table2D<unsigned int> ui_copy = ui_2DTable;

will have the pointer m_2DTable pointing to the same memory in all 3. If one goes out of scope, the memory will be deleted, while the others will still try to access it - if you're lucky, this will crash. I won't go into complete detail on how to fix this, but you can read about the so called "Rule of Three" here. Suffice to say, the version using std::map doesn't suffer from this problem (one of the nice benefits of standard containers is this comes "for free").

\$\endgroup\$
  • \$\begingroup\$ This post deserve + 1 and above. \$\endgroup\$ – Noobie Mar 15 '13 at 6:51
  • \$\begingroup\$ std::cout << f_2DTable.get(10, 10) << "\n"; will cause an undefined behavior right? Hm, how would I resolve that? \$\endgroup\$ – Noobie Mar 15 '13 at 6:53
  • \$\begingroup\$ @Noobie You need to do some bounds checking. Check if x >= m_SizeX || y >= m_SizeY and either do something like throw std::out_of_range or return some kind of default value. \$\endgroup\$ – Yuushi Mar 15 '13 at 6:58
  • \$\begingroup\$ Thank you @Yuushi. Your post taught and helped me a lot. Thank you. Now, I need to go study the Rule of Three and Five. :) \$\endgroup\$ – Noobie Mar 15 '13 at 6:59

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.