Question

The delete statement below is a code redundancy.

int *a = new int[100]
if (!doSomething1(a))
   delete[] a
   return
if (!doSomething2(a))
   delete[] a
   return
if (!doSomething3(a))
   delete[] a
   return

return

One alternative I came up with is:

if(doSomething1(a) || doSomething2(a) || doSomething3(a))
   ;
delete[] a;
return;

But it does not fit in situation where I want to add instructions specified to doSomething1(), e.g.

if(!doSomething1(a))
   foo;
   delete[] a;
   return;
if(!doSomething2(a))
   delete[] a;
   return;

I was told there are at least 5 ways to reduced this kind of code redundancy. So any suggestions?


update, one more question: same question, Let's limit the domain to C only, note process logic change.

char *a = (char*)malloc(100*sizeof(char));
if(doSomething1(a))
   foo;
   free(a)
   return;
if(doSomething2(a))
   free(a)
   return;
if(doSomething3(a))
   free(a);
   return;
free(a);
return
Was it helpful?

Solution

Why do you need to manually delete anything? This is C++, so we can use destructors and RAII.

std::vector<a>(100);
if (!doSomething1(a))
    return;
if (!doSomething2(a))
    return;
if (!doSomething3(a))
    return;

More generally: if anything needs to happen when you leave a scope, then do it in the destructor of an automatic object in that scope.

But why are you using return values to indicate failure? This is C++, so we can use exceptions:

std::vector<a>(100);
doSomething1(a);
doSomething2(a);
doSomething3(a);

OTHER TIPS

"RAII" and "scope exit" would be good search terms for Google or SO. For example, look at Boost.ScopeExit or the alternatives to it. The idea is that you create a local object and leave it to the destructor to do the cleanup or "finally" code.

Here's an attempt at applying the idiom directly to your example, reduced to the bare minimum (no error checking and stuff like that):

class AutomaticDeleter
{
public:
    AutomaticDeleter(int *a) : a(a) {}
    ~AutomaticDeleter() { delete[] a; }
    int *GetWrappedPointer() const { return a; }
private:
    int *a;
};

// ...

AutomaticDeleter automatic_deleter(new int[100]);

if (!doSomething1(automatic_deleter.GetWrappedPointer()))
   return;
if (!doSomething2(automatic_deleter.GetWrappedPointer()))
   return;
if (!doSomething3(automatic_deleter.GetWrappedPointer()))
   return;

And of course, I should mention that this is just an example of what you can do, not of what you should do when applying the idiom to a pointer, especially one returned from new[]. In real life, you'd use std::vector<int> and be done with it. The idiom has much more interesting and useful applications.

bool failure;

int *a = new int[100]
if ( failure = !doSomething1(a))
   //...
if ( !failure && ( failure = !doSomething2(a) ) )
   //...
if ( !failure && ( failure =!doSomething3(a) ) )
   //...

if ( failure )
{
   delete []a;
   return;
}
//,,,   

return
do {
    if (doSomething1(a) && doSomething2(a) && doSomething3(a)){
        break; /*all good: move outside the dummy loop*/
    }
    /*one of the functions failed*/
    delete[] a;
    return;
} while (false);

is one way. I'm relying on the fact that if evaluates statements strictly from left to right and will stop evaluation once a false is reached.

(This idiom is used quite frequently in C; see unix kernel source code).

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top