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").