Critique this c++ code.

Question

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)->setSomething(10);
    bp->process();
}

Answer 1

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.

Answer 2

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

Answer 3

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. ;)

Answer 4

There's one actual error and a bunch of dangerous/questionable practices:

---

The one error is that you never call delete on your newed object, so it leaks.

---

Questionable practices:

1. Base doesn't have a virtual destructor, so if you correct the error by calling delete or using an auto_ptr, you'll invoke undefined behaviour.
2. There's no need to use dynamic allocation here at all.
3. Polymorphic base classes should be uncopyable to prevent object slicing.
4. You're using a dynamic_cast where it's not neccessary and without checking the result - why not just declare bp as a pointer to ProductANew or ProductNew?
5. 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.