CODEWITHSUNDEEP
c++
Jagannath
Jagannath

Reputation: 4025

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

Upvotes: 1

Views: 492

Answers (4)

JoeG
JoeG

Reputation: 13192

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.

Upvotes: 3

JUST MY correct OPINION
JUST MY correct OPINION

Reputation: 36107

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

Upvotes: 1

anon
anon

Reputation:

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

Upvotes: 19

sharptooth
sharptooth

Reputation: 170499

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.

Upvotes: 7

Related Questions