943,957 Members | Top Members by Rank

Ad:
  • C++ Discussion Thread
  • Marked Solved
  • Views: 953
  • C++ RSS
Dec 27th, 2008
0

Excercise in class design - code review

Expand Post »
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!

C++ Syntax (Toggle Plain Text)
  1. #ifndef _UTILITY_NAMEVALUEPAIR_H_
  2. #define _UTILITY_NAMEVALUEPAIR_H_
  3.  
  4. #include <sstream>
  5. using std::ostringstream;
  6.  
  7. #include <string>
  8. using std::string;
  9.  
  10. namespace Utility
  11. {
  12. template<typename T>
  13. class NameValuePair
  14. {
  15. public:
  16. static const float Version;
  17.  
  18. NameValuePair(const string&, const T&);
  19. NameValuePair(const NameValuePair<T>&);
  20. virtual ~NameValuePair();
  21.  
  22. virtual const NameValuePair<T>& operator =(const NameValuePair<T>&);
  23. virtual bool operator ==(const NameValuePair<T>&) const;
  24. virtual bool operator !=(const NameValuePair<T>&) const;
  25.  
  26. virtual const string& getName() const;
  27. virtual void setName(const string&);
  28.  
  29. virtual const T& getValue() const;
  30. virtual void setValue(const T&);
  31.  
  32. virtual const string ToString() const;
  33.  
  34. private:
  35. string _name;
  36. T _value;
  37.  
  38. NameValuePair(){};
  39. };
  40.  
  41. // Version
  42. template <typename T>
  43. const float NameValuePair<T>::Version = 0.1f;
  44.  
  45. // Life Cycle
  46. template <typename T>
  47. NameValuePair<T>::NameValuePair(const string& name, const T& value)
  48. {
  49. _name = name;
  50. _value = value;
  51. }
  52.  
  53. template <typename T>
  54. NameValuePair<T>::NameValuePair(const NameValuePair<T>& pair)
  55. {
  56. _name = pair._name;
  57. _value = pair._value;
  58. }
  59.  
  60. template <typename T>
  61. NameValuePair<T>::~NameValuePair()
  62. {
  63. }
  64.  
  65. // Operators
  66. template <typename T>
  67. const NameValuePair<T>& NameValuePair<T>::operator =(const NameValuePair<T>& rhs)
  68. {
  69. if (rhs != *this)
  70. {
  71. _name = rhs._name;
  72. _value = rhs._value;
  73. }
  74. return *this;
  75. }
  76.  
  77. template <typename T>
  78. bool NameValuePair<T>::operator ==(const NameValuePair<T>& rhs) const
  79. {
  80. return ((_name == rhs._name) && (_value == rhs._value));
  81. }
  82.  
  83. template <typename T>
  84. bool NameValuePair<T>::operator !=(const NameValuePair<T>& rhs) const
  85. {
  86. return !(*this == rhs);
  87. }
  88.  
  89. // Properties (getter/setters)
  90. // Name
  91. template <typename T>
  92. const string& NameValuePair<T>::getName() const
  93. {
  94. return _name;
  95. }
  96.  
  97. template <typename T>
  98. void NameValuePair<T>::setName(const string& name)
  99. {
  100. _name = name;
  101. }
  102.  
  103. // Value
  104. template <typename T>
  105. const T& NameValuePair<T>::getValue() const
  106. {
  107. return _value;
  108. }
  109.  
  110. template <typename T>
  111. void NameValuePair<T>::setValue(const T& value)
  112. {
  113. _value = value;
  114. }
  115.  
  116. // Public Methods
  117. template <typename T>
  118. const string NameValuePair<T>::ToString() const
  119. {
  120. ostringstream stringBuilder;
  121. stringBuilder << _name << "=" << _value;
  122. string returnString = stringBuilder.str();
  123. return returnString;
  124. }
  125. }
  126.  
  127. #endif
Reputation Points: 13
Solved Threads: 7
Light Poster
tymk is offline Offline
35 posts
since Apr 2008
Dec 27th, 2008
0

Re: Excercise in class design - code review

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.
Reputation Points: 2035
Solved Threads: 645
Senior Poster
ddanbe is offline Offline
3,740 posts
since Oct 2008
Dec 27th, 2008
0

Re: Excercise in class design - code review

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?
Reputation Points: 13
Solved Threads: 7
Light Poster
tymk is offline Offline
35 posts
since Apr 2008
Dec 27th, 2008
0

Re: Excercise in class design - code review

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 ...
Reputation Points: 1234
Solved Threads: 347
Postaholic
ArkM is offline Offline
2,001 posts
since Jul 2008
Dec 27th, 2008
0

Re: Excercise in class design - code review

Click to Expand / Collapse  Quote originally posted by ArkM ...
Alas, my conclusion is: too many defects.
Thanks, this is exactly the type of feedback I'm looking for.
Click to Expand / Collapse  Quote originally posted by ArkM ...
1. I have never seen float type version number. It's cool!
Really? Is this one of your defects? If so, why?

Click to Expand / Collapse  Quote originally posted by ArkM ...
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.

Click to Expand / Collapse  Quote originally posted by ArkM ...
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.

Click to Expand / Collapse  Quote originally posted by ArkM ...
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.

Click to Expand / Collapse  Quote originally posted by ArkM ...
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?

Click to Expand / Collapse  Quote originally posted by ArkM ...
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.


Click to Expand / Collapse  Quote originally posted by ArkM ...
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?

Click to Expand / Collapse  Quote originally posted by ArkM ...
8. No comments or rudimentary useless comments.
Noted.

Click to Expand / Collapse  Quote originally posted by ArkM ...
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!
Reputation Points: 13
Solved Threads: 7
Light Poster
tymk is offline Offline
35 posts
since Apr 2008
Dec 28th, 2008
0

Re: Excercise in class design - code review

#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++.
Reputation Points: 1234
Solved Threads: 347
Postaholic
ArkM is offline Offline
2,001 posts
since Jul 2008
Dec 29th, 2008
0

Re: Excercise in class design - code review

Click to Expand / Collapse  Quote originally posted by ArkM ...
#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!"
Click to Expand / Collapse  Quote originally posted by ArkM ...
#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.
Click to Expand / Collapse  Quote originally posted by ArkM ...
#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?
Reputation Points: 13
Solved Threads: 7
Light Poster
tymk is offline Offline
35 posts
since Apr 2008
Dec 29th, 2008
0

Re: Excercise in class design - code review

Click to Expand / Collapse  Quote originally posted by tymk ...
...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
Reputation Points: 1234
Solved Threads: 347
Postaholic
ArkM is offline Offline
2,001 posts
since Jul 2008
Dec 30th, 2008
0

Re: Excercise in class design - code review

Ah, ok, I get it now. Thank you for all of your help!
Reputation Points: 13
Solved Threads: 7
Light Poster
tymk is offline Offline
35 posts
since Apr 2008

This thread is solved

Either the thread starter or a moderator has marked this thread as solved. You can most likely trust the responses and answers given. There is most likely no reason for any further responses to be posted here. If you have a related question, please start a new thread in this forum instead.

This thread is more than three months old

No one has posted to this discussion for at least three months. Please let old threads die and do not reply to them unless you feel you have something new and valuable to contribute that absolutely must be added to make the discussion complete. Otherwise, please start a new thread in this forum instead.
Message:
Previous Thread in C++ Forum Timeline: dllexport for all functions
Next Thread in C++ Forum Timeline: need help with prime





About Us | Contact Us | Advertise | Acceptable Use Policy
Forum Index | Build Custom RSS Feed


Follow us on Twitter


© 2011 DaniWeb® LLC