Critique this c++ code
Similar to the code written below exists in production. Could you people review it and tell me if such code works well all the time.
class Base
{
public:
virtual void process() = 0;
};
class ProductA : public Base
{
public:
void process()
{
// some implementation.
doSomething();
}
void setSomething(int x)
{
}
virtual void doSomething()
{
// doSomething.
}
};
class ProductANew : public ProductA
{
public:
ProductANew() : ProductA() { }
void doSomething()
{
// do Something.
}
};
int main(int argc, char *argv[])
{
Base* bp = new ProductANew();
dynamic_cast<ProductA*>(bp)开发者_Go百科->setSomething(10);
bp->process();
}
Some problems:
- the base class must have a virtual destructor
- you never delete the object allocated with new
- you never test the result of dynamic_cast
With good design you wouldn't need a dynamic_cast
. If process()
can't be called without calling setSomething()
first they should have been exposed in the same base class.
There's one actual error and a bunch of dangerous/questionable practices:
The one error is that you never call delete
on your new
ed object, so it leaks.
Questionable practices:
Base
doesn't have a virtual destructor, so if you correct the error by callingdelete
or using anauto_ptr
, you'll invoke undefined behaviour.- There's no need to use dynamic allocation here at all.
- Polymorphic base classes should be uncopyable to prevent object slicing.
- You're using a
dynamic_cast
where it's not neccessary and without checking the result - why not just declarebp
as a pointer toProductANew
orProductNew
? ProductANew
doesn't need a constructor - the default one will do just fine.
A few of these points may be a result of the nature of your example - i.e. you have good reason to use dynamic allocation, but you wanted to keep your example small.
Generally you'll find that code which can't even compile is badly designed.
Base* bp = new ProductANew();
This line can't work because ProductANew isn't inherited from Base in any way, shape or form.
$ gcc junk.cc
junk.cc: In function ‘int main(int, char**)’:
junk.cc:41: error: cannot convert ‘ProductANew*’ to ‘Base*’ in initialization
(Just to be clear: junk.cc contains your code cut and pasted.)
Edited to add...
Latecomers may want to look at the history of the original question before down-voting. ;)
精彩评论