Where to put "delete" in the loop

Question

Suppose I have the following code:

int decreasingTest(int number){
int numberOfDigits = 0; do { number /= 10; numberOfDigits++; } while (number != 0);
int *arrayValue = new int[numberOfDigits + 1];
for(int i = numberOfDigits;0<=i;i--){ //acquire digits of number
    arrayValue[i] = (number - fmod(number, pow(10, i))) / pow(10, i);
    number = fmod(number, pow(10, i));
    i--;
}
for (int j = 0; j < sizeOfArray; j++){
    if (arrayValue[j + 1] <= arrayValue[j]){
        if ((j + 1) == sizeOfArray){
            cout << "The number's digits are decreasing." << endl;
            return 5;
        }
        continue;
    }
    return 0;
}

}

Should I put delete arrayValue[] in the line before return 0 and after the bracket after continue, after the line return 0, or on the line before the last bracket?

Answer 1

There are three places in your code where you might return from the function:

1. on the return 5 line
2. on the return 0 line, after the loop
3. Before the closing brace, if the outer loop never executes.

In order to not leak resources, there must be one delete call executed for every new call executed. In your case, since you have two return points, you need two delete calls -- one just before each of the return points.

Manually managing dynamic memory as you're doing here is problematic. It's very easy to forget to delete something, especially when you have multiple return points. Many language features and library components have been built to solve this problem. Smart pointers like shared_ptr and unique_ptr manage the destruction of dynamically-allocated resources automatically so you don't need to write delete at all, and make_shared and make_unique enable you to never even write new. (Note: make_unique isn't in the C++11 Standard, but it's easy to write, and it is expected to be in the C++14 Standard.)

C-Style arrays are also problematic in their own right for a number of reasons. One of the most obvious is their fixed-size and the magic numbers that often come with them. Language features and library components exist to solve this problems as well, and I'd suggest that as a rule, you should (almost) never use C-style arrays. Instead, use a vector.

In your case, you don't need pointers at all. You just need a `vector':

std::vector <int> arrayValue;
for(int i = numberOfDigits;0<=i;i--){ //acquire digits of number
    arrayValue.push_front(number - fmod (number, pow(10, i))) / pow(10,i);
    //arrayValue[i] = (number - fmod(number, pow(10, i))) / pow(10, i);
    number = fmod(number, pow(10, i));
    i--;
}

...

Answer 2

For this particular case, there is no reason to use dynamic memory. Not trying to be pedantic, but dynamic memory gets used lots of places it should not be.

[ADDENDUM (further information and shameless grab for possible points)]

You'd think the needed number of digits would already be in a standard library, since decimal representation is so common. The following is based on a posting by Hallvard Furuseth in Google Group comp.std.c, but I do not deduct the sign bit from the initial bit-size count, so it should work for both signed and unsigned types. The +3 term is for the digit needed to store factor '1' (log10(1)=0); for the sign '+' or '-'; and for a possible null termination character. The factor of 146/485 is only slightly greater than log10(2) (equivalently log(2)/log(10) in any base), which is the digit scaling factor from binary to decimal representation. Please let me know if you spot any problems.

//Overestimate for number of decimal digits representing an integer of type integerType
//Works for both signed and unsigned integer types
//Includes space for sign ('+' or '-') and for a termination character
//Upper-bounds the function floor(log10(abs(n))) + 3

#define SIZEDIGITS(integerType) (((sizeof(integerType)*CHAR_BIT)*146)/485 + 3)

int8_t: 5 digits ('+/-127\0')

uint8_t: 5 digits ('+255\0')

APPROXIMATION gives (8*146)/485 + 3 = 5 digits

int16_t: 7 digits ('+/-32767\0')

uint16_t: 7 digits ('+65536\0')

APPROXIMATION gives (16*146)/485 + 3 = 7 digits

int32_t: 12 digits ('+/-2147483647\0')

uint32_t: 12 digits ('+4294967295\0')

APPROXIMATION gives (32*146)/485 + 3 = 12 digits

int64_t: 21 digits ('+/-9223372036854775807\0')

uint64_t: 22 digits ('+18446744073709551615\0')

APPROXIMATION gives (64*146)/485 + 3 = 22 digits

int128_t: 41 digits

uint128_t: 41 digits

APPROXIMATION gives (128*146)/485 + 3 = 41 digits

int9999_t (pretend): 3012 digits

uint9999_t (pretend): 3012 digits

APPROXIMATION gives (9999*146)/485 + 3 = 3013 (an unused byte - the humanity!)

(Random bonus fact you may or may not be aware of: base-e is considered the most efficient real-number base in terms of our usual sum-of-powers digit representation for real numbers. Ternary outperforms binary in this respect, since it's closer to e.)

Answer 3

1. The most important point is that there is almost never a case in which you should new an array anymore. I would strongly suggest that you use a std::vector here, so your int *arrayValue = new int[numberOfDigits + 1]; would become: std::vector<int> arrayValue(numberOfDigits + 1); And you're done no other code changes needed.
   
   Note that in situations where the code can error, say one of your pows throws for instance and you leave this function without returning, std::vector still cleans itself up, something that cannot be said of newed memory.

2. If you find yourself in the 1% of situations where you should manage your own memory. A good rule of thumb is never have more than one return statement. It simply makes your code unreadable and unmaintainable. So you should change your last loop to:

   int result = 0;
int j = 0;
   

   while(j < sizeOfArray - 1 && arrayValue[j + 1] <= arrayValue[j]){
j++;
}
   

   if (j == sizeOfArray - 1){
cout << "The number's digits are decreasing." << endl;
result = 5;
}
delete[] arrayValue;
return result;
   

Answer 4

It is the case when you should use standard smart pointer std::unique_ptr declared in header <memory>. For example

std::unique_ptr<int[]> arrayValue( new int[numberOfDigits + 1] );

In this case you need not to bother about deleting the array.

Answer 5

Given the code structure you have now, there are three separate places that you would need to put your delete [] arrayValue; command (note, the [] is before arrayValue, not after it). You would need to put it on the line right above return 5;, on the line right above return 0;, and right before the last bracket. It is possible that the code exits your function on any of those three lines, and, in this case, you should deallocate your memory right before you exit the function.

Alternatively, you could just replace the line int *arrayValue = new int[numberOfDigits + 1]; with std::vector<int> arrayValue(numberOfDigits + 1), and let the compiler handle the allocation/deallocation for you.

Answer 6

You could alternatively use a smart pointer, for example std::unique_ptr (http://en.cppreference.com/w/cpp/memory/unique_ptr) which automatically frees the dynamically allocated array when the pointer goes out of scope.