Re: completed my Varaint class.
From: Arthur J. O'Dwyer (ajo_at_nospam.andrew.cmu.edu)
Date: 01/19/04
- Next message: Joec: "Re: Difficult question."
- Previous message: Joec: "Re: Difficult question."
- In reply to: Jumbo: "completed my Varaint class."
- Next in thread: Martijn Lievaart: "Re: completed my Varaint class."
- Reply: Martijn Lievaart: "Re: completed my Varaint class."
- Reply: Jumbo: "Re: completed my Varaint class."
- Reply: Chris Val: "Re: completed my Varaint class."
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Date: Mon, 19 Jan 2004 16:01:39 -0500 (EST)
On Mon, 19 Jan 2004, Jumbo wrote:
>
> Hi there,
> I have completed the variant class I have been working on and I wonder if
> anyone could try and break it for me to see if there's anything I have
> overlooked or screwed up on.
The only real compiler errors are pretty trivial; things like
encroaching on the implementation's namespace or forgetting to return
a value from a function. The design of the program is what's really
messed up -- it seems like a really ad-hoc and non-extensible method
of getting a heterogeneous array. In real life, you wouldn't need
these classes in the first place; and if you wanted to play with the
heterogeneous-array idea anyway, you'd probably look through the
[non-standard but quite popular] Boost library.
> It is a wee bit long for the casual reader so please be warned:
It's longer with my comments interspersed. ;-)
> Here's the class etc:
> /* User defined type simply for testing purposes */
> class UD{
> public:
> UD(){std::cout<<"UD constructor"<<'\n';}
> };
>
> #ifndef _STRING_
> #include <string>
> #endif
Two errors: first, the dependence on the #definedness of an identifier
which is /a priori/ out of your control (_STRING_ is part of the
implementation's namespace and thus cannot be depended upon to have any
useful value at any point in the program); and second, the way you're
only #including <string> if some identifier has *not* been #defined.
Occasionally, conditional compilation might see a programmer write
#ifdef USE_STDSTRING
#include <string>
#endif
but this #ifndef thing is weird, and thus probably wrong. I think you
meant to write
#include <string>
> #ifndef _VECTOR_
> #include <vector>
> #endif
Ditto: lose the #ifndef...#endif.
> /* forward declaration*/
> class DataType;
>
> class Base{
> public:
> virtual ~Base()=0{}
> virtual void setData(int){}
> virtual void setData(double){}
> virtual void setData(std::string){}
> virtual void setData(UD){}
> };
Here's another couple of problems, although these are more design-
oriented. First, note that your class can *by design* accept only
ints, doubles, strings, and UDs. What happens when the user wants
to use a heterogeneous array containing floats or chars or complexes
or FooBarBazzes? He's just out of luck, right?
Secondly, there's no need to use inheritance for this project.
You could lose the 'Base' class entirely, and not have any problems,
as far as I can tell. The inheritance gains you nothing.
> class Variant{
> public:
> Variant():itsArray(0),itsLen(0){}
Presumably you are going to add some 'resize' methods to this
class once you get it working -- otherwise this constructor makes
no sense. (If not, you might as well explicitly make 'itsLen'
a constant value.)
Also, you should really consider the benefits of std::vector.
Managing a bunch of individual 'Base *' objects in an array will
quickly become tedious.
> Variant(const std::vector<std::string> &v1);
> ~Variant();
> Variant(const Variant& rhs);
> Variant& operator=(const Variant& rhs);
> size_t getLen()const{return itsLen;}
Have you #included <cstdlib> or <stdlib.h> and I didn't notice,
or is 'size_t' defined somewhere else in C++?
Also, why 'getLen' and not 'length', to mimic the interface of
C++'s standard vector class, 'std::vector'? Making your user
remember two names for the same concept is a Bad Thing.
> template<typename T>
> void setData(int ind, T val){
> if(ind >=0 && ind<itsLen)
> if(itsArray[ind])itsArray[ind]->setData(val);
> }
>
> template<typename T>
> T getData(int ind){
> if(ind >=0 && ind<itsLen)
> if(itsArray[ind])
> return static_cast<DataType<T>* >(itsArray[ind])->getData();
...else, return what? You should have received a compiler diagnostic
for this error; it should not have slipped into your final code. I
suggest searching Google Groups for ways to portably return a 'nothing'
value from a templated function; or else just throwing an exception.
But if you leave it like this, both the 'if's are wasted -- you get
undefined behavior anyway.
> }
>
> private:
> std::vector<std::string> itsTypes;
> size_t itsLen;
> Base** itsArray;
Vector for 'itsTypes', but array for 'itsArray'? That's weird...
> template<typename T>
> class DataType: public Base{
> public:
> DataType():data(0){}
> ~DataType(){}
> void setData(T val){data=val;}
> T getData(){return data;}
> private:
> T data;
> };
> };
>
> /* Specialized DataType constructors*/
> template<>
> Variant::DataType<std::string>::DataType():data("0"){}
> /* resolves error caused if std::string is initialised to 0*/
Why "0" and not ""?
> template<>
> Variant::DataType<UD>::DataType(){}
> /* resolves error caused if std::string is initialised to 0*/
This comment makes no sense. Bad commenting is even worse
than no commenting.
> /* Variant constructor */
> Variant::Variant(const std::vector<std::string> &v1)
> :itsArray(new Base*[v1.size()]),
> itsLen(v1.size()),
> itsTypes(v1)
> {
> for(size_t i=0;i<v1.size();i++){
Nitpick: ITYM 'std::vector<std::string>::size_type i = 0; ...'
[which I think can be improved to legibility by adding a typedef
inside this function, but I'm not absolutely sure about C++ typedef
scoping rules].
> if(v1[i]=="int")itsArray[i] = new DataType<int>;
> else if(v1[i]=="double")itsArray[i] = new DataType<double>;
> else if(v1[i]=="std::string")itsArray[i] = new DataType<std::string>;
> else if(v1[i]=="UD")itsArray[i] = new DataType<UD>;
> else itsArray[i] = 0;
You place much trust in your users. I would never expect the user
to spell perfectly, or to guess that "std::string" was the correct
way to refer to 'string' even after the user had explicitly declared
that he was 'using std::string;'. This is a design problem that
won't be solved by hacking around in this function alone; a good
program would have a nice robust method of handling user error.
> }
> }
>
> /* Variant copy constructor */
> Variant::Variant(const Variant& rhs)
> :itsLen(rhs.getLen()),
> itsArray(new Base*[itsLen]),
> itsTypes(rhs.itsTypes)
> {
> for(size_t i=0;i<itsLen;i++){
> if( rhs.itsTypes[i]=="int"){
> itsArray[i] = new DataType<int>;
> itsArray[i]->setData(static_cast<DataType<int>*
> >(rhs.itsArray[i])->getData());
> }
> else if(rhs.itsTypes[i]=="double"){
> itsArray[i] = new DataType<double>;
> itsArray[i]->setData(static_cast<DataType<double>*
> >(rhs.itsArray[i])->getData());
> }
> else if(rhs.itsTypes[i]=="std::string"){
> itsArray[i] = new DataType<std::string>;
> itsArray[i]->setData(static_cast<DataType<std::string>*
> >(rhs.itsArray[i])->getData());
> }
> else if(rhs.itsTypes[i]=="UD"){
> itsArray[i] = new DataType<UD>;
> itsArray[i]->setData(static_cast<DataType<UD>*
> >(rhs.itsArray[i])->getData());
> }
> else itsArray[i] = 0;
> }
> }
Ditto this function: yuck!
> /* Variant destructor */
> Variant::~Variant(){
> for(size_t i=0;i<itsLen;i++){
> delete itsArray[i];
> }
> delete itsArray;
Bug: you meant 'delete [] itsArray;' This is one of C++'s biggest
pitfalls, IMHO, since only the cleverest compilers can catch even
the most common occurrences of this bug.
> itsArray =0;
> }
>
> /* Variant operator= */
> Variant& Variant::operator=(const Variant& rhs){
> itsTypes =rhs.itsTypes;
> if(itsLen != rhs.getLen()){
> this->~Variant();
> itsLen = rhs.getLen();
> itsArray = new Base*[itsLen];
> for(size_t i=0;i<itsLen;i++){
> if( rhs.itsTypes[i]=="int")itsArray[i] = new DataType<int>;
> else if(rhs.itsTypes[i]=="double")itsArray[i] = new DataType<double>;
> else if(rhs.itsTypes[i]=="std::string")itsArray[i] = new
> DataType<std::string>;
> else if(rhs.itsTypes[i]=="UD")itsArray[i] = new DataType<UD>;
> else itsArray[i] = 0;
> }
> }
> for(size_t i=0;i<itsLen;i++){
These two loops should be executed in parallel inside a single loop,
not split up like this. Write instead,
for (size_t i=0; i < itsLen; ++i) {
if (rhs.itsTypes[i] == "int") {
itsArray[i] = new DataType<int>;
itsArray[i]->setData(static_cast<DataType<int>*>
(rhs.itsArray[i]->getData());
}
else if (rhs.itsTypes[i] == "double") {
[...]
> if( rhs.itsTypes[i]=="int")
> itsArray[i]->setData(static_cast<DataType<int>*
> >(rhs.itsArray[i])->getData());
> else if(rhs.itsTypes[i]=="double")
> itsArray[i]->setData(static_cast<DataType<double>*
> >(rhs.itsArray[i])->getData());
> else if(rhs.itsTypes[i]=="std::string")
> itsArray[i]->setData(static_cast<DataType<std::string>*
> >(rhs.itsArray[i])->getData());
> else if(rhs.itsTypes[i]=="std::string")
> itsArray[i]->setData(static_cast<DataType<UD>*
> >(rhs.itsArray[i])->getData());
> }
> return *this;
> }
>
> Right, that's all the hard stuff out of the way , I saved it in my include
> folder an named it vector.
Huh?
> Now here's a little test program to demonstrate its usage:
>
> #include <iostream>
> #include <variant>
>
> int main(){
> std::vector<std::string> strArr1(5);
> strArr1[0]= "int";
> strArr1[1]= "std::string";
> strArr1[2]= "double";
> strArr1[3] = "UD";
...and strArr1[4] equals...? You invoke undefined behavior later,
by trying to evaluate the contents of strArr1[4] without having put
anything there.
> Variant v1(strArr1);
> v1.setData(0,5);
> v1.setData(1,"Hello");
> v1.setData(2, 6.5);
> v1.setData(3, 5); /* does nothing to UD */
>
> std::vector<Variant> vv(2);
It's also kind of weird that here you define a vector of two
Variants, but you only use one of them. Sure, it's a test
program, but you weren't seriously thinking that your class might
break if put into a vector, were you??
> vv[0] = v1;
>
> std::cout<< vv[0].getData<int>(0) <<'\n';
> std::cout<< vv[0].getData<std::string>(1) <<'\n';
> std::cout<< vv[0].getData<double>(2) <<'\n';
> vv[0].getData<UD>(3);
Ouch! Here's the biggest problem, which I hadn't even been
considering until it was right in my face: You're setting up this
big, complicated data structure with inheritance and string
comparisons and template functions and all that, and after all
that work, you're still making your client remember what types
he's stored in the array?! That's just mean!
I could get exactly the same behavior you've demonstrated here
in five minutes with a managed vector of (void*). And so could
anyone who wanted to -- including yourself! So what purpose does
this code serve, other than an example of uber-bad practice?
> /*try to break it etc etc*/
> return 0;
> }
>
> Obviously the class is basic and would need to be adapted for project
> specific use.
> If anyone can spot any errrors please let me know.
See the previous sentence: bad design is itself an error.
> Also be obliged if anyone can see a way to overload the subscripts so as to
> return different types.
I'm sure Boost has some clever way to minimize the client's workload,
but I'm not at all sure that C++ lets you do this; it is, after all, a
statically typed language.
> TIA.
You're welcome.
-Arthur
- Next message: Joec: "Re: Difficult question."
- Previous message: Joec: "Re: Difficult question."
- In reply to: Jumbo: "completed my Varaint class."
- Next in thread: Martijn Lievaart: "Re: completed my Varaint class."
- Reply: Martijn Lievaart: "Re: completed my Varaint class."
- Reply: Jumbo: "Re: completed my Varaint class."
- Reply: Chris Val: "Re: completed my Varaint class."
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Relevant Pages
|
