CODEWITHSUNDEEP
c++
Rajendra Uppal
Rajendra Uppal

Reputation: 19964

Do you see any kind of problem with this C++ code?

class A
{
public:
    A(){}
    ~A(){}
    void DoSomething(int x){}   
};

void func(int i)
{
    A *pa = new A();
    pa->DoSomething(i);
    delete pa;
}

Do you guys see any problem with this code? I can only see following two:

  1. func is only operating on object of class A and should be made member of A.
  2. object of class A should be created on the stack instead of heap.

Any other ideas?

Upvotes: 0

Views: 157

Answers (4)

fredoverflow
fredoverflow

Reputation: 263380

Is this real code? Then the stateless class is completely superfluous, and void DoSomething(int x) should be a normal function. If not, post the real code, otherwise it's hard to give further advice.

Upvotes: 0

James McNellis
James McNellis

Reputation: 355367

object of class A should be created on the stack instead of heap.

Yes, pa should be created as an automatic variable (on the stack) instead of dynamically (on the heap).

However, it's also wrong as written because it isn't exception-safe. If pa->DoSomething(i) throws an exception, you will leak the object pointed to by pa.

The correct way to manage resource lifetimes is to use Scope-Bound Resource Management (SBRM; also called Resource Acquisition Is Initialization). The RAII way to manage a dynamically allocated object is to use a smart pointer:

void func(int i)
{
    std::auto_ptr<A> pa(new A());
    pa->DoSomething(i);
}

Manual resource management is brittle and dangerous because it's easy to get wrong. In this trivial example, it's easy to see that pa->DoSomething(i) does not throw an exception because it doesn't do anything at all. But in almost every real program, it's not that easy.

Manual resource management quickly becomes very difficult as a program grows in size and complexity. Automatic resource management using Scope-Bound Resource Management scales very well.

func is only operating on object of class A and should be made member of A.

This is not correct. You should only implement a function as a member function if it requires access to the internal state of the object. This means that you should prefer to implement functions as non-member functions wherever possible.

The more member functions you have, the more work it takes to thoroughly test a class because there are more ways that the internal state of the class can be modified.

Herb Sutter explains this principle by breaking down the std::string class in his Guru of the Week article "Monoliths Unstrung."

Upvotes: 9

dennycrane
dennycrane

Reputation: 2331

Also, a class that needs a copy constructor, an assignment operator or a destructor typically needs all three (I know, this one is empty - just saying).

Upvotes: 0

riderchap
riderchap

Reputation: 687

Any exception happens on func() while executing A::DoSomething() cause a memory leak. Use a smart pointer (e.g. std::auto_ptr)

Upvotes: 2

Related Questions