same operation in multiple if statement, how to reduce code redundancy

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

Mike Seymour · Accepted Answer · 2014-03-12T16:56:19.537

2

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

edited Mar 12 '14 at 16:56

answered Mar 12 '14 at 16:30

Mike Seymour

249,747
28
448
644

Your points are valid, but I think his/her question was about control structures, not memory allocation. I think the memory allocation was just a generic stand-in for whatever code. – Iron Savior Mar 12 '14 at 16:36
Yesss!!! Exceptions, so *nobody* will see the twisted control flow behind the code. Dijkstra and your "goto considered harmful", we can do much, much worse! Take that!! **(Evil laughter echoing)** – vonbrand Mar 12 '14 at 16:39
@IronSavior: The answer is the same in the general case: if something has to be done when leaving the current scope, do it in the destructor of a scoped automatic object. – Mike Seymour Mar 12 '14 at 16:40
@vonbrand: Or, to put it another way: exceptions, so *nobody* can ignore an error condition and carry on into undefined behaviour. After three or so decades of mainstream use, most people don't regard their flow as "twisted". – Mike Seymour Mar 12 '14 at 16:51
@MikeSeymour You are correct and I agree this is the most appropriate answer. Your first sentence was a comment about memory management and since the asker may not actually have a memory management problem, he/she may discount your answer without seeing the wisdom in RAII. – Iron Savior Mar 12 '14 at 16:53
@MikeSeymour What does 'twisted control flow' mean, exception chain? – Tianwei Chen Mar 13 '14 at 03:04
@MikeSeymour I have updated one more question, do you mind to take a look? – Tianwei Chen Mar 13 '14 at 11:25
@Sky: I'm a C++ programmer, and this is a C++ question. If you have a question about a different language, ask a separate question with the appropriate language tag. – Mike Seymour Mar 13 '14 at 12:06

score 1 · Answer 2 · answered Mar 12 '14 at 16:36

"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.

This is a nice explanation, I couldn't fully appreciate MikeSeymou's answer without it. However, since MikeSeymour covers more information(but not that plain to the person asking this kind of question), I'll chose MikeSeymours as accepted answer. — Tianwei Chen, Mar 13 '14 at 03:00
@Sky: not a problem at all. I've been digitally socialised in Usenet, where reputation was not expressed as a number displayed next to a nickname. I must admit I like the SO system, but I know one should not get carried away with it :) — Christian Hackl, Mar 13 '14 at 16:06

Bathsheba · Answer 3 · 2014-03-12T16:42:37.210

0

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

edited Mar 12 '14 at 16:42

answered Mar 12 '14 at 16:24

Bathsheba

231,907
34
361
483

1

Personally, I'd rather see a real `goto` than a weirdly disguised one; and I'd rather see C++ idioms than those from a different language. – Mike Seymour Mar 12 '14 at 16:32
@MikeSeymour: A `goto` requires your using a label for starters; the `do` `while` is cleaner in that respect. – Bathsheba Mar 12 '14 at 16:38
It's not that `if` evaluates statements strictly from left to right. This is a non-sense. It's that logic operators are short-circuited. – bolov Mar 12 '14 at 16:41
@bolov: added, in simple terminology. – Bathsheba Mar 12 '14 at 16:42

score 0 · Answer 4 · answered Mar 12 '14 at 16:30

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

same operation in multiple if statement, how to reduce code redundancy

4 Answers4