Hi all. I wanted to run this class by some experienced programmers to see if I have any obvious errors and to get any advice otherwise.
I created a test suite and everything seems to function properly so I'm interested in feedback on overall class design, choice of data types, const correctness, not-so-obvious gotcha!s, etc. Thanks!

#ifndef _UTILITY_NAMEVALUEPAIR_H_
#define _UTILITY_NAMEVALUEPAIR_H_

#include <sstream>
using std::ostringstream;

#include <string>
using std::string;

namespace Utility
{
	template<typename T>
	class NameValuePair
	{
	public:
		static const float Version;

		NameValuePair(const string&, const T&);
		NameValuePair(const NameValuePair<T>&);
		virtual ~NameValuePair();

		virtual const NameValuePair<T>& operator =(const NameValuePair<T>&);
		virtual bool operator ==(const NameValuePair<T>&) const;
		virtual bool operator !=(const NameValuePair<T>&) const;

		virtual const string& getName() const;
		virtual void setName(const string&);

		virtual const T& getValue() const;
		virtual void setValue(const T&);

		virtual const string ToString() const;

	private:
		string _name;
		T _value;

		NameValuePair(){};
	};
	
	// Version
	template <typename T>
	const float NameValuePair<T>::Version = 0.1f;
	
	// Life Cycle
	template <typename T>
	NameValuePair<T>::NameValuePair(const string& name, const T& value)
	{
		_name = name;
		_value = value;
	}
	
	template <typename T>
	NameValuePair<T>::NameValuePair(const NameValuePair<T>& pair)
	{
		_name = pair._name;
		_value = pair._value;
	}
	
	template <typename T>
	NameValuePair<T>::~NameValuePair()
	{
	}
	
	// Operators
	template <typename T>
	const NameValuePair<T>& NameValuePair<T>::operator =(const NameValuePair<T>& rhs)
	{
		if (rhs != *this)
		{
			_name = rhs._name;
			_value = rhs._value;
		}
		return *this;
	}
	
	template <typename T>
	bool NameValuePair<T>::operator ==(const NameValuePair<T>& rhs) const
	{
		return ((_name == rhs._name) && (_value == rhs._value));
	}

	template <typename T>
	bool NameValuePair<T>::operator !=(const NameValuePair<T>& rhs) const
	{
		return !(*this == rhs);
	}

	// Properties (getter/setters)
	// Name
	template <typename T>
	const string& NameValuePair<T>::getName() const
	{
		return _name;
	}
	
	template <typename T>
	void NameValuePair<T>::setName(const string& name)
	{
		_name = name;
	}

	// Value
	template <typename T>
	const T& NameValuePair<T>::getValue() const
	{
		return _value;
	}
	
	template <typename T>
	void NameValuePair<T>::setValue(const T& value)
	{
		_value = value;
	}

	// Public Methods
	template <typename T>
	const string NameValuePair<T>::ToString() const
	{
		ostringstream stringBuilder;
		stringBuilder << _name << "=" << _value;
		string returnString = stringBuilder.str();
		return returnString;
	}
}

#endif

Recommended Answers

All 8 Replies

Nice.
But why let the value be generic and the name not? Or was that the intention in the first place in which case I have said nothing.

Thank you. Yes, a non-generic name member was my intent for this excercise. I wanted to start with something simple before I tackle a larger project. One thing I overlooked was error handling. Not sure where it could fit here, any advice?

Alas, my conclusion is: too many defects.
1. I have never seen float type version number. It's cool!
2. I don't understand why nobody can declare arrays of NameValuePair (default constructor is private). Everybody can assign arbitrary values to both pair members via setters but it's impossible to define default member values - a very strange project decision.
3. No need in explicit copy constructor and assignment operator definition: they follows by default copy constructor and assignment operator logics (do it member by member).
4. The toString() member function requires operator<<(string,type) for a template argument type but template class itself does not redefine its own operator<< for std::string. Why?
5. I consider system heaser includes and using directives in user .h files as a very bad and dangerous practice.
6. It's a good practice to place simplest template member function definitions in the class definition or declare them as inline explicitly. Ignored.
7. This class has a virtual destructor (considered as a base of) but it has no protected members: unusual and suspicious symptom.
8. No comments or rudimentary useless comments.

I think it's not a professionally designed class template. I prefer std::pair till now ;)...

Alas, my conclusion is: too many defects.

Thanks, this is exactly the type of feedback I'm looking for.

1. I have never seen float type version number. It's cool!

Really? Is this one of your defects? If so, why?

2. I don't understand why nobody can declare arrays of NameValuePair (default constructor is private). Everybody can assign arbitrary values to both pair members via setters but it's impossible to define default member values - a very strange project decision.

I suppose I took the lazy way out. I got as far as thinking I could make objects default to null, but not standard types so I gave up and just made the default constructor private. Any advice regarding this would be appreciated.

3. No need in explicit copy constructor and assignment operator definition: they follows by default copy constructor and assignment operator logics (do it member by member).

Understood.

4. The toString() member function requires operator<<(string,type) for a template argument type but template class itself does not redefine its own operator<< for std::string. Why?

I suppose I omitted it and will add it.

5. I consider system heaser includes and using directives in user .h files as a very bad and dangerous practice.

Not sure how to avoid this in a template class? Especially if following the advice in #6 below? Why dangerous?

6. It's a good practice to place simplest template member function definitions in the class definition or declare them as inline explicitly. Ignored.

Noted.

7. This class has a virtual destructor (considered as a base of) but it has no protected members: unusual and suspicious symptom.

Yes, probably should make these members protected. Suspicious symptom?

8. No comments or rudimentary useless comments.

Noted.

I think it's not a professionally designed class template. I prefer std::pair till now ;)...

I didn't expect it to be professional on my first try, that's why I asked for feedback! Wasn't trying to replace std::pair for you either, just trying to get some practice designing classes the correct way. Thank you for taking a look and sharing your opinion!

#1. Version 1.2 means "subversion 2 of version 1", not "version 1 + 2/10". The only reliable floating point "is equal" op is "value == 0.0".
#2. You can't declare type arrays if no accessible default constructor for the type. I can't understand why it's so dangerous to declare arrays of pairs.
#5. You open (at least a part of) outside namespace for user module code. That's obviously undesired side effect of such includes. Use qualified names: std::string, std::ostringstream etc if your template has STL dependencies. Document these dependencies. Let the template user provide all system headers for his code.
>Not sure how to avoid this in a template class?
And what's a problem?

Yet another remark. I think such "pair" templates must have two typename parameters for both members. It's so easy to get a proper template specialization for std::string as the key member type.

Conclusion: a pair of unrelated values template is not an interesting generic class because no non-trivial operations for this type. So it's not so good base to study generic programming in C++.

#1. Version 1.2 means "subversion 2 of version 1", not "version 1 + 2/10". The only reliable floating point "is equal" op is "value == 0.0".

Thanks, this is much more honest, direct, informative and helpful than "It's cool!"

#2. You can't declare type arrays if no accessible default constructor for the type. I can't understand why it's so dangerous to declare arrays of pairs.

OK, now I see what you mean.

#5. You open (at least a part of) outside namespace for user module code. That's obviously undesired side effect of such includes. Use qualified names: std::string, std::ostringstream etc if your template has STL dependencies. Document these dependencies. Let the template user provide all system headers for his code.
>Not sure how to avoid this in a template class?
And what's a problem?

I understand eliminating the using std::string but I don't understand eliminating the #include <string> . Would the preferred practice be a forward declaration of std::string?

...but I don't understand eliminating the #include <string> . Would the preferred practice be a forward declaration of std::string?

Why not? Let a user includes all needed system headers then includes all third-party .h files then its own .h files... It looks like a honest, simple and logical solution: define system environment then define workbench environment then local environment...

That's why I prefer explicit namespace qualification in all user .h files: let this .h-file user makes decision where to place sacramental (and very dangerous) using namespace std; or avoid it.

It's also a pragmatically authorized practice. For example, it's a good practice in MS VC++ to place ALL needed system headers in the project stdafx.h file (MS VC++ pre-compiled headers container). If so, no need to include system headers in user code at all.

Worst case: your template user gets a simple and clear compiler message about std::string error. OK, he/she inserts #include <string> before include_template_definition and gets a wonderful value pair class depended on std::string ;)

Ah, ok, I get it now. Thank you for all of your help!

Be a part of the DaniWeb community

We're a friendly, industry-focused community of developers, IT pros, digital marketers, and technology enthusiasts meeting, networking, learning, and sharing knowledge.