C++ Template DataTable Class Need Feedback

[101MUDman101]

Yo, damn what a bad way to start

Anyway In my 2D Engine I am developing I have made my own Data Table system, I should probably explain what it does. It is a template based class that creates a virtual table with 2 columns (DataLabel, DataValue), from there you can add rows of data, here is the code snippet, you are free to use this at your own will:

Code: Select all

template<class DataLabel, class DataValue> 
class DataTable {
private:
	const char* name;
	std::map<DataLabel, DataValue> Reg;
public:
	DataTable(const char* tablelabel);
	void AddElement(DataLabel label, DataValue value);
	void RemoveElement(DataLabel label);
	void ChangeLabel(DataLabel oldlabel, DataLabel newlabel);
	void ChangeValue(DataLabel label, DataValue newvalue);
	const char* GetTableLabel();
	DataLabel GetDataLabel(DataLabel label);
	DataValue GetDataValue(DataLabel label);
	bool DoesLabelExist(DataLabel label);
	bool DoesValueExist(DataLabel label, DataValue value);
};

template <class DataLabel, class DataValue>
DataTable<DataLabel, DataValue>::DataTable(const char* tablelabel) {
	name = tablelabel;
}

template<class DataLabel, class DataValue>
void DataTable<DataLabel, DataValue>::AddElement(DataLabel label, DataValue value) {
	Reg[label] = value;
}

template<class DataLabel, class DataValue>
void DataTable<DataLabel, DataValue>::RemoveElement(DataLabel label) {
	Reg.erase(label);
}

template<class DataLabel, class DataValue>
void DataTable<DataLabel, DataValue>::ChangeLabel(DataLabel oldlabel, DataLabel newlabel) {
	if (Reg[oldlabel] && newlabel != oldlabel) {
		Reg[newlabel] = Reg[oldlabel];
		Reg.erase(oldlabel);
	}
}

template<class DataLabel, class DataValue>
void DataTable<DataLabel, DataValue>::ChangeValue(DataLabel label, DataValue newvalue) {
	if (Reg[label] && newvalue != Reg[label]) {
		Reg[label] = newvalue;
	}
}

template<class DataLabel, class DataValue>
DataLabel DataTable<DataLabel, DataValue>::GetDataLabel(DataLabel label) {
	if (Reg[label]) {
		return label;
	}

	return NULL;
}

template<class DataLabel, class DataValue>
const char* DataTable<DataLabel, DataValue>::GetTableLabel() {
	return name;
}

template<class DataLabel, class DataValue>
DataValue DataTable<DataLabel, DataValue>::GetDataValue(DataLabel label) {
	if (Reg[label]) {
		return Reg[label];
	}

	return 0;
}

template<class DataLabel, class DataValue>
bool DataTable<DataLabel, DataValue>::DoesLabelExist(DataLabel label) {
	if (Reg[label]) {
		return true;
	}
	else {
		return false;
	}
}

template<class DataLabel, class DataValue>
bool DataTable<DataLabel, DataValue>::DoesValueExist(DataLabel label, DataValue value) {
	if (Reg[label]) {
		if (Reg[label] == value) {
			return true;
		} else {
			return false;
		}
	} else {
		return false;
	}
}

Here's a demo of the class in use:

Code: Select all

//Lets make a table that holds our friends names and ages
DataTable<const char*, int> MyFriends("Friends");
MyFriends.AddElement("Bob", 17);
MyFriends.AddElement("Fred", 12);
MyFriends.AddElement("Emma", 22);
//What if Fred had a birthday?
MyFriends.ChangeDataValue("Fred", MyTable.GetDataValue("Fred")+1));
//What if Emma changed her name to Bob? Lol
MyFriends.ChangeDataLabel("Emma", MyFriends.GetDataLabel("Bob"));
//Say we had a massive table and wanted to know if David existed?
if (MyFriends.DoesLabelExist("David")) == true {
    cout << "David doesn't exist! OOOHHHH NOOEEESSS!" << endl;
}

What do you guys think?

Feedback Suggestions: Have I wrote the code keeping in mind good programming practices are have I just pooped a pile of words?

[dandymcgee]

So it's a std::map wrapper? Are you trying to emulate a key/value store?

[101MUDman101]

So it's a std::map wrapper? Are you trying to emulate a key/value store?

Along the lines of that, but I've found this is much easier to use. You can use any datatypes for the DataLabel and DataValue, you could check whether specific files are open by making DataLabel hold fstreams and the DataValue hold booleans.

[101MUDman101]

I just think this is an efficient way of handling data, as you can check whether values/labels exist and such. I think this would me best for people that need to store temporary data but don't want to go through the hassle of setting up an std::map/vector.

[Nokurn]

This class will not work the way you expect it to.
You claim that it's an efficient way of handling data (misuse of the word efficient--you meant easy-to-use) because you can check whether values/labels exist. Your code code not check whether values/labels exist, it checks if they evaluate to true.

Code: Select all

if (Reg[label]) // This is how you are checking for existence
// What if the DataTable maps strings to booleans? label is a string, Reg[label] is a boolean
// Say that "Bool1" is mapped to false
// Reg["Bool1"] will return false, because that is its value, not its existence

The only reason this code works in a sense is that when you call the [] operator on a std::map, it will create the item if one with the given key isn't found. The initial value is the default-constructed value type. Some examples:

Code: Select all

int i(); // Will be 0
bool b(); // Will be false

These evaluate to false. Which makes your code "work" unless the item does exist and its value really does evaluate to false.

If you want to actually check for existence of a key, use iterators.

Code: Select all

template<class TKey, class TValue>
bool containsKey(std::map<TKey, TValue> const& m, TKey const& k)
{
    return m.find(k) != m.end();
}

To check for the existence of a value (note: doesn't care if there's more than one; if you need unique values, use std::unique_map):

Code: Select all

// You could use a lambda instead of a functor
template<class TKey, class TValue>
struct MapValuePredicate
{
    TValue const& v;
    MapValuePredicate(TValue const& v_) : v(v_) {}
    bool operator()(std::pair<TKey, TValue> const& p) const { return p.second == v; }
};

template<class TKey, class TValue>
bool containsValue(std::map<TKey, TValue> const& m, TValue const& v)
{
    return std::find_if(m.begin(), m.end(), MapValuePredicate(v)) != m.end();
}

As for the hassle, I honestly don't know what you mean.

The functions I've just given you will properly check for key/value existence in a std::map. You can use them as-is on any std::map--and, with a little work, std::unordered_map. You could wrap a std::map in a class that contains these methods, but it would honestly contain a lot of superfluous code as well as adding another layer of abstraction (which your complier might stupidly obey, leaving you with pointless overhead).

[101MUDman101]

This class will not work the way you expect it to.
You claim that it's an efficient way of handling data (misuse of the word efficient--you meant easy-to-use) because you can check whether values/labels exist. Your code code not check whether values/labels exist, it checks if they evaluate to true.

Code: Select all

if (Reg[label]) // This is how you are checking for existence
// What if the DataTable maps strings to booleans? label is a string, Reg[label] is a boolean
// Say that "Bool1" is mapped to false
// Reg["Bool1"] will return false, because that is its value, not its existence

The only reason this code works in a sense is that when you call the [] operator on a std::map, it will create the item if one with the given key isn't found. The initial value is the default-constructed value type. Some examples:

Code: Select all

int i(); // Will be 0
bool b(); // Will be false

These evaluate to false. Which makes your code "work" unless the item does exist and its value really does evaluate to false.

If you want to actually check for existence of a key, use iterators.

Code: Select all

template<class TKey, class TValue>
bool containsKey(std::map<TKey, TValue> const& m, TKey const& k)
{
    return m.find(k) != m.end();
}

To check for the existence of a value (note: doesn't care if there's more than one; if you need unique values, use std::unique_map):

Code: Select all

// You could use a lambda instead of a functor
template<class TKey, class TValue>
struct MapValuePredicate
{
    TValue const& v;
    MapValuePredicate(TValue const& v_) : v(v_) {}
    bool operator()(std::pair<TKey, TValue> const& p) const { return p.second == v; }
};

template<class TKey, class TValue>
bool containsValue(std::map<TKey, TValue> const& m, TValue const& v)
{
    return std::find_if(m.begin(), m.end(), MapValuePredicate(v)) != m.end();
}

As for the hassle, I honestly don't know what you mean.

The functions I've just given you will properly check for key/value existence in a std::map. You can use them as-is on any std::map--and, with a little work, std::unordered_map. You could wrap a std::map in a class that contains these methods, but it would honestly contain a lot of superfluous code as well as adding another layer of abstraction (which your complier might stupidly obey, leaving you with pointless overhead).

For gods sake it's a first draft

Thanks for the feedback, this is exactly the kind of advice I wanted, I salute you sir!

[Falco Girgis]

I'm surprised nobody has mentioned the other issue... You have a little problem with the const char * case, homeskillet.

Try this:

Code: Select all

const char *bob1 = "Bob";
const char *bob2 = "Bob";
DataTable<const char*, int> MyFriends("Friends");
MyFriends.AddElement(bob1, 17);
MyFriends.AddElement(bob2, 12);

I'm guessing you would expect this to create one "Bob" entry with the value of 12. Unfortunately, you will have two entries.

This is because your key equality comparisons are not being made on the basis of the contents of the character array. They are being made based on the memory address of the character array. Since bob1 != bob2 (simple pointer comparison), the two entries are not the same.

This is the exact reason the STL has an an optional comparator functor argument for std::map. You have to pass your map a comparison functor that does a string compare rather than using the default "==" behavior.

[101MUDman101]

So I would have to pass a reference of bob1 and bob2 to the AddElement function? I have rewrite some of it so that it now iterates through the map to find the value and not just get the address.

[Light-Dark]

This is the exact reason the STL has an an optional comparator functor argument for std::map. You have to pass your map a comparison functor that does a string compare rather than using the default "==" behavior.

Key Compare/value compare
Also here's one for comparing your average cstring(const char*)
http://www.cplusplus.com/reference/clib ... ng/strcmp/

[101MUDman101]

Thanks for all the help! I have finally sorted the class out. Here is the updated class and functions:

Code: Select all

template<class DataLabel, class DataValue> 
class DataTable {
private:
	const char* name;
	std::map<DataLabel, DataValue> Reg;
	typename std::map<DataLabel, DataValue>::iterator it;
	typename std::map<DataLabel, DataValue>::key_compare RegComp;
	std::pair<DataLabel, DataValue> last; //Key/Value of last element
public:
	DataTable(const char* tablelabel);
	void AddElement(DataLabel label, DataValue value);
	void RemoveElement(DataLabel label);
	void ChangeLabel(DataLabel oldlabel, DataLabel newlabel);
	void ChangeValue(DataLabel label, DataValue newvalue);
	const char* GetTableLabel();
	DataLabel GetDataLabel(DataLabel label);
	DataValue GetDataValue(DataLabel label);
	bool DoesLabelExist(DataLabel const& k);
	bool DoesValueExist(DataValue const& v);
};

Code: Select all

template<class DataLabel, class DataValue>
bool DataTable<DataLabel, DataValue>::DoesLabelExist(DataLabel const& k) {
	it = Reg.begin();
	do {
		if (k == (*it).first) {
			return true;
		}
	} while ( RegComp((*it++).first, last.first) );

	return false;
}

template<class DataLabel, class DataValue>
bool DataTable<DataLabel, DataValue>::DoesValueExist(DataValue const& v) {
	it = Reg.begin();
	do {
		if (v == (*it).second) {
			return true;
		}
	} while ( Reg.value_comp()(*it++, last) );

	return false;
}

I really hope this is right

[qpHalcy0n]

Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.

Who in the HELL ever said that?!

[dandymcgee]

Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.

Who in the HELL ever said that?!

More like, if you write the code as cleverly as possible, there is no need to debug it. :P

[101MUDman101]

Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it.

Who in the HELL ever said that?!

More like, if you write the code as cleverly as possible, there is no need to debug it. :P

Brian W. Kernighan

Though if you write the code as terribly as possible and debugged it successfully, wouldn't that make you a clever programmer?