6
\$\begingroup\$

I have answered a Question in Stackoverflow link.

a) Create a function called resize that can be used to increase the size of integer arrays dynamically. The function takes three parameters. The first parameter is the original array, the second parameter is the size of this array, and the third parameter is the size of the larger array to be created by this function. Make sure that you allocate memory from the heap inside this function. After allocating memory for the second array the function must copy the elements from the first array into the larger array. Finally, the function must return a pointer to the new array.

b. In main, allocate an array on the heap that is just large enough to store the integers 5, 7, 3, and 1.

c. Resize the array to store 10 integers by calling the resize function created in step a. Remove the old (smaller) array from the heap. Add the numbers 4, 2, and 8 to the end of the new array.

d. Write a sort function that sorts any integer array in increasing order.

e. Use the sort function to sort the array of numbers in c above. Display the sorted numbers.

Is there a Dangling pointer issue.

#include <array>
#include <iostream>

void swap(int *xp, int *yp) 
{ 
    int temp = *xp; 
    *xp = *yp; 
    *yp = temp; 
} 

//Bubble Sort
bool sort(int arr[], int size)
{
    for( int i = 0; i< size -1; i++)
    {
        for( int j = 0; j < size - i -1; j++)
        {
            //descending order
            if(arr[j]<arr[j+1])
            {
                swap(&arr[j], &arr[j+1]);
            }
        }
    }
    return true;
}

void Print(int Array[], int nSize)
{
    for( int i = 0; i < nSize; i++)
    {
        std::cout<<" "<<Array[i];
    }
    std::cout<<"\n";
}

void Resize( int *&Array, const int& nSizeOld, const int& nSize )
{
    int * newArray = new int[nSize];

    //Copy Elements of the Array
    for(int i = 0; i< nSize; i++)
    {
        newArray[i] = Array[i];
    }

    delete[] Array;

    //Assign ptr of Prev to new Array
    Array =  newArray;
}

int _tmain(int argc, _TCHAR* argv[])
{
    const int kNewSize = 10, kSize = 5;
    int *pMyArray = new int[kSize];

    //Set Values
    for( int i = 0; i< kSize; ++i ) 
    {
        pMyArray[i] = i * 5;
    }

    Resize( pMyArray, kSize, kNewSize );

    //Set Values
    for( int i = kSize; i< kNewSize; ++i ) 
    {
        pMyArray[i] = i * 10;
    }

    Print(pMyArray, kNewSize);

    sort(pMyArray, kNewSize);

    Print(pMyArray, kNewSize);

    if( pMyArray!=NULL )
    {
        delete[] pMyArray;
    }

    return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ As Roland Illig points out below, in C++ there's no need to implement your own low level memory management for this when you can just use std::vector instead. That said, if you do need/want to implement your own low level array resizing, you should be using std::realloc(). The reference page I linked to even provides some (to my inexpert eye) decent example code. \$\endgroup\$ – Ilmari Karonen Apr 24 at 11:56
  • \$\begingroup\$ @IlmariKaronen In fact, you pretty much shouldn't be using realloc in C++ code ... \$\endgroup\$ – L. F. Apr 27 at 1:40
18
\$\begingroup\$

If you had tagged this code as C, it would have been acceptable. Since you tagged it as C++, it's horrible.

Instead of writing your own swap function, there's already std::swap in <algorithm>.

Instead of writing bubble sort yourself, just use std::sort, also from <algorithm>.

Instead of using arrays and resizing them yourself, just use std::vector<int>, from <vector>.

After applying these transformations, you cannot have a dangling pointer anymore since your code is completely pointer-free.

As part of an exercise for learning the basic operations on memory management, it's ok to write code like this, but never ever use such code in production. In production the code should look like this:

#include <algorithm>
#include <iostream>
#include <vector>

void Print(const std::vector<int> &nums)
{
    for(int num : nums)
    {
        std::cout << " " << num;
    }
    std::cout << "\n";
}

int main()
{
    std::vector<int> nums { 5, 7, 3, 1 };

    // There's probably a more elegant way to add the elements to the vector.
    nums.push_back(4);
    nums.push_back(2);
    nums.push_back(8);

    std::sort(nums.begin(), nums.end());

    Print(nums);
}

By the way, your original code doesn't have any dangling pointer as well. Well done.

You don't need the != NULL check before the delete[] since that pointer cannot be null. In modern C++ (since C++11 I think) you would also write nullptr instead of NULL. The reason is that historically NULL had not been guaranteed to be of pointer type.

Have a look at https://en.cppreference.com/w/cpp/algorithm for more algorithms that you shouldn't implement yourself in C++.

I would have liked to write the push_back block in a shorter way, as well as the Print function. I'm sure there's a more elegant way, I just don't know it.

\$\endgroup\$
  • \$\begingroup\$ I would just stream char-literals instead of length-one string-literals. And cppreference.com is inho a better reference. \$\endgroup\$ – Deduplicator Apr 24 at 11:45
  • \$\begingroup\$ @Deduplicator I changed the link, thanks for the hint. Regarding the single characters: shouldn't the compiler produce exactly the same code for these two variants by detecting the length 1 string? I thought this was possible in C++. I like code that is as uniform as possible, and using string literals for anything string-like seems simple and nice to me. \$\endgroup\$ – Roland Illig Apr 24 at 18:33
  • \$\begingroup\$ The compiler could in theory inline enough layers to get the same code in the end using whole-program-analysis, necessary due to virtual functions called. I doubt it though. \$\endgroup\$ – Deduplicator Apr 24 at 19:15
  • \$\begingroup\$ "You don't need the != NULL check before the delete[] since that pointer cannot be null" The behaviour is identical anyway, as the delete and delete[] operators are both defined to be null safe (in which case they do nothing). \$\endgroup\$ – Max Barraclough Apr 25 at 21:57
  • \$\begingroup\$ Thanks for the Review \$\endgroup\$ – f1r361rd Apr 29 at 13:00
4
\$\begingroup\$

The code is obviously wrong: your compiler should have warmed you that Resize() never uses its nSizeOld parameter.

\$\endgroup\$
  • 1
    \$\begingroup\$ That's a funny concept of "wrong"... Anyway, it was probably a typo, since it actually is needed; the question was edited accordingly. This answer is superfluous now. \$\endgroup\$ – Anakhand Apr 24 at 6:58
  • 2
    \$\begingroup\$ Please see What to do when someone answers. I have rolled back Rev 2 → 1. \$\endgroup\$ – 200_success Apr 24 at 7:09
  • 4
    \$\begingroup\$ If we follow those rules literally, to correct [what seems to me] a simple typo, OP should ask a completely new question with the fix. Seems kind of ridiculous for such a trivial matter... If every question followed that rule verbatim this site would me a mess. I'm not saying your comment isn't correct or helpful, but it should be precisely that, a comment, so that OP can fix the [irrelevant] issue and move on. \$\endgroup\$ – Anakhand Apr 24 at 8:11
  • 2
    \$\begingroup\$ While your answer is correct and you made a good observation I'd contend that this type of error (a trivial bug) was not what the OP wanted to ask (it would, after all, be OT), so that it does not warrant an answer. I'd have written a comment and waited for the correction. You should imo reinstate the corrected version of the question and delete (or preface as obsolete) your answer. \$\endgroup\$ – Peter A. Schneider Apr 24 at 17:10
  • 1
    \$\begingroup\$ @PeterA.Schneider As per the rules in the help center, all aspects of the code are open to critique. This out-of-bounds memory access bug (which may or may not have been apparent to the author) is absolutely fair game for a Code Review answer. \$\endgroup\$ – 200_success Apr 24 at 17:17
2
\$\begingroup\$
  1. Your code is too low-level. It expresses implementation details instead of intent. That's why your code looks like "C with couts instead of printf and new/delete instead of malloc/free" instead of C++.

  2. Roland Illig has already told you that you should use std::swap instead of building a new one from scratch. You should use existing libraries, especially the standard library, whenever possible.

    That said, your own implementation of swap is also questionable. This is C++, not C. We have references. Using pointers makes the code less readable, and puts burden on the user of the function. So you should change it to:

    void swap(int& x, int& y)
    {
        int temp = x;
        x = y;
        y = temp;
    }
    

    And the calls to it can be changed from swap(&foo, &bar) to swap(foo, bar). Still, std::swap is preferable.

  3. Again, Roland Illig has already told you that you should use the std::sort instead of building a new bubble sort from scratch. std::sort typically uses quicksort, which has \$O(n \log n)\$ time complexity; whereas bubble sort has \$O(n^2)\$ time complexity. It should be obvious that std::sort is much more efficient.

  4. Your parameter lists are so C-ish. (pointer, size) parameter pairs are everywhere. They are error-prone. Consider using spans. (Spans are currently not available in the standard library; consider using the one from GSL)

    You even have parameter lists like (int*& Array, const int& nSizeOld, const int& nSize). Don't pass by const reference for builtin types. Just pass by value, as in int nSizeOld, int nSize. And letting a pointer denote an array with sizes littered everywhere holds a great welcome party for errors.

  5. Don't use _tmain and _TCHAR. They are not portable. (Strictly speaking, they are not proper C++.) You should write in ISO standard C++. Use main and char instead.

    // Correct prototype of the main function
    int main(int argc, char* argv[])
    {
        // ...
    }
    
  6. Don't make such liberal use of "naked" news and deletes. Explicit calls to news and deletes are very error prone. std::vectors should be preferred from the beginning.

  7. You have four for loops in total. The first three use i++, whereas the last one uses ++i. Please consistently use ++i.

As a conclusion: you should refactor your code to express intent.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the Review. \$\endgroup\$ – f1r361rd Apr 29 at 13:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.