Array Dynamic resize in heapO(lg n) algorithm for a fibonacci number with c++ templateNecklace counting problem-with consecutive prime constraintCounting words, letters, average word length, and letter frequencyC++ object poolUse of containers for storing StudentsC# to C++ function call that fills and auto resize array if size is not enoughGeneric Template implementation of merge sort in C++Binary searching the turning point of a functionSymbolic algebra using a generic smart pointer classC++11 min-heap with configurable arity
Why doesn't WotC use established keywords on all new cards?
Purpose of のは in this sentence?
Has a commercial or military jet bi-plane ever been manufactured?
How do I make a function that generates nth natural number that isn't a perfect square?
How to apply differences on part of a list and keep the rest
How can I close a gap between my fence and my neighbor's that's on his side of the property line?
BOOM! Perfect Clear for Mr. T
Hyperlink on red background
As matter approaches a black hole, does it speed up?
Expressing 'our' for objects belonging to our apartment
Independent, post-Brexit Scotland - would there be a hard border with England?
Can a nothic's Weird Insight action discover secrets about a player character that the character doesn't know about themselves?
Are there any Final Fantasy Spirits in Super Smash Bros Ultimate?
What is the difference between 'unconcealed' and 'revealed'?
What is the name of this hexagon/pentagon polyhedron?
I have a unique character that I'm having a problem writing. He's a virus!
How long would it take for people to notice a mass disappearance?
How can I get a job without pushing my family's income into a higher tax bracket?
What are the differences between credential stuffing and password spraying?
Can there be a single technologically advanced nation, in a continent full of non-technologically advanced nations?
Send iMessage from Firefox
How did Kirk identify Gorgan in "And the Children Shall Lead"?
What property of a BJT transistor makes it an amplifier?
Would glacier 'trees' be plausible?
Array Dynamic resize in heap
O(lg n) algorithm for a fibonacci number with c++ templateNecklace counting problem-with consecutive prime constraintCounting words, letters, average word length, and letter frequencyC++ object poolUse of containers for storing StudentsC# to C++ function call that fills and auto resize array if size is not enoughGeneric Template implementation of merge sort in C++Binary searching the turning point of a functionSymbolic algebra using a generic smart pointer classC++11 min-heap with configurable arity
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty margin-bottom:0;
$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;
c++ c++11 pointers
$endgroup$
add a comment |
$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;
c++ c++11 pointers
$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 usestd::vector
instead. That said, if you do need/want to implement your own low level array resizing, you should be usingstd::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 usingrealloc
in C++ code ...
$endgroup$
– L. F.
Apr 27 at 1:40
add a comment |
$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;
c++ c++11 pointers
$endgroup$
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;
c++ c++11 pointers
c++ c++11 pointers
edited Apr 24 at 7:08
200_success
132k20159424
132k20159424
asked Apr 24 at 5:33
f1r361rdf1r361rd
365
365
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 usestd::vector
instead. That said, if you do need/want to implement your own low level array resizing, you should be usingstd::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 usingrealloc
in C++ code ...
$endgroup$
– L. F.
Apr 27 at 1:40
add a comment |
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 usestd::vector
instead. That said, if you do need/want to implement your own low level array resizing, you should be usingstd::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 usingrealloc
in C++ code ...
$endgroup$
– L. F.
Apr 27 at 1:40
1
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$
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
$begingroup$
@IlmariKaronen In fact, you pretty much shouldn't be using
realloc
in C++ code ...$endgroup$
– L. F.
Apr 27 at 1:40
add a comment |
3 Answers
3
active
oldest
votes
$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 thedelete
anddelete[]
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
2 days ago
add a comment |
$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
add a comment |
$begingroup$
Your code is too low-level. It expresses implementation details instead of intent. That's why your code looks like "C with
cout
s instead ofprintf
andnew
/delete
instead ofmalloc
/free
" instead of C++.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)
toswap(foo, bar)
. Still,std::swap
is preferable.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 thatstd::sort
is much more efficient.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 inint nSizeOld
,int nSize
. And letting a pointer denote an array with sizes littered everywhere holds a great welcome party for errors.Don't use
_tmain
and_TCHAR
. They are not portable. (Strictly speaking, they are not proper C++.) You should write in ISO standard C++. Usemain
andchar
instead.
// Correct prototype of the main function
int main(int argc, char* argv[])
// ...Don't make such liberal use of "naked"
new
s anddelete
s. Explicit calls tonew
s anddelete
s are very error prone.std::vector
s should be preferred from the beginning.You have four
for
loops in total. The first three usei++
, 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
2 days ago
add a comment |
Your Answer
StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");
StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);
else
createEditor();
);
function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader:
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
,
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);
);
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f218994%2farray-dynamic-resize-in-heap%23new-answer', 'question_page');
);
Post as a guest
Required, but never shown
3 Answers
3
active
oldest
votes
3 Answers
3
active
oldest
votes
active
oldest
votes
active
oldest
votes
$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 thedelete
anddelete[]
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
2 days ago
add a comment |
$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 thedelete
anddelete[]
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
2 days ago
add a comment |
$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$
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.
edited Apr 24 at 18:29
answered Apr 24 at 6:19
Roland IlligRoland Illig
12.4k12050
12.4k12050
$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 thedelete
anddelete[]
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
2 days ago
add a comment |
$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 thedelete
anddelete[]
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
2 days ago
$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$
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$
@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$
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$
"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
2 days ago
$begingroup$
Thanks for the Review
$endgroup$
– f1r361rd
2 days ago
add a comment |
$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
add a comment |
$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
add a comment |
$begingroup$
The code is obviously wrong: your compiler should have warmed you that Resize()
never uses its nSizeOld
parameter.
$endgroup$
The code is obviously wrong: your compiler should have warmed you that Resize()
never uses its nSizeOld
parameter.
answered Apr 24 at 6:37
200_success200_success
132k20159424
132k20159424
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
add a comment |
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
1
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
$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
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
$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
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
$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
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
$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
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
$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
add a comment |
$begingroup$
Your code is too low-level. It expresses implementation details instead of intent. That's why your code looks like "C with
cout
s instead ofprintf
andnew
/delete
instead ofmalloc
/free
" instead of C++.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)
toswap(foo, bar)
. Still,std::swap
is preferable.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 thatstd::sort
is much more efficient.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 inint nSizeOld
,int nSize
. And letting a pointer denote an array with sizes littered everywhere holds a great welcome party for errors.Don't use
_tmain
and_TCHAR
. They are not portable. (Strictly speaking, they are not proper C++.) You should write in ISO standard C++. Usemain
andchar
instead.
// Correct prototype of the main function
int main(int argc, char* argv[])
// ...Don't make such liberal use of "naked"
new
s anddelete
s. Explicit calls tonew
s anddelete
s are very error prone.std::vector
s should be preferred from the beginning.You have four
for
loops in total. The first three usei++
, 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
2 days ago
add a comment |
$begingroup$
Your code is too low-level. It expresses implementation details instead of intent. That's why your code looks like "C with
cout
s instead ofprintf
andnew
/delete
instead ofmalloc
/free
" instead of C++.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)
toswap(foo, bar)
. Still,std::swap
is preferable.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 thatstd::sort
is much more efficient.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 inint nSizeOld
,int nSize
. And letting a pointer denote an array with sizes littered everywhere holds a great welcome party for errors.Don't use
_tmain
and_TCHAR
. They are not portable. (Strictly speaking, they are not proper C++.) You should write in ISO standard C++. Usemain
andchar
instead.
// Correct prototype of the main function
int main(int argc, char* argv[])
// ...Don't make such liberal use of "naked"
new
s anddelete
s. Explicit calls tonew
s anddelete
s are very error prone.std::vector
s should be preferred from the beginning.You have four
for
loops in total. The first three usei++
, 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
2 days ago
add a comment |
$begingroup$
Your code is too low-level. It expresses implementation details instead of intent. That's why your code looks like "C with
cout
s instead ofprintf
andnew
/delete
instead ofmalloc
/free
" instead of C++.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)
toswap(foo, bar)
. Still,std::swap
is preferable.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 thatstd::sort
is much more efficient.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 inint nSizeOld
,int nSize
. And letting a pointer denote an array with sizes littered everywhere holds a great welcome party for errors.Don't use
_tmain
and_TCHAR
. They are not portable. (Strictly speaking, they are not proper C++.) You should write in ISO standard C++. Usemain
andchar
instead.
// Correct prototype of the main function
int main(int argc, char* argv[])
// ...Don't make such liberal use of "naked"
new
s anddelete
s. Explicit calls tonew
s anddelete
s are very error prone.std::vector
s should be preferred from the beginning.You have four
for
loops in total. The first three usei++
, whereas the last one uses++i
. Please consistently use++i
.
As a conclusion: you should refactor your code to express intent.
$endgroup$
Your code is too low-level. It expresses implementation details instead of intent. That's why your code looks like "C with
cout
s instead ofprintf
andnew
/delete
instead ofmalloc
/free
" instead of C++.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)
toswap(foo, bar)
. Still,std::swap
is preferable.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 thatstd::sort
is much more efficient.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 inint nSizeOld
,int nSize
. And letting a pointer denote an array with sizes littered everywhere holds a great welcome party for errors.Don't use
_tmain
and_TCHAR
. They are not portable. (Strictly speaking, they are not proper C++.) You should write in ISO standard C++. Usemain
andchar
instead.
// Correct prototype of the main function
int main(int argc, char* argv[])
// ...Don't make such liberal use of "naked"
new
s anddelete
s. Explicit calls tonew
s anddelete
s are very error prone.std::vector
s should be preferred from the beginning.You have four
for
loops in total. The first three usei++
, whereas the last one uses++i
. Please consistently use++i
.
As a conclusion: you should refactor your code to express intent.
answered Apr 27 at 2:12
L. F.L. F.
31012
31012
$begingroup$
Thanks for the Review.
$endgroup$
– f1r361rd
2 days ago
add a comment |
$begingroup$
Thanks for the Review.
$endgroup$
– f1r361rd
2 days ago
$begingroup$
Thanks for the Review.
$endgroup$
– f1r361rd
2 days ago
$begingroup$
Thanks for the Review.
$endgroup$
– f1r361rd
2 days ago
add a comment |
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f218994%2farray-dynamic-resize-in-heap%23new-answer', 'question_page');
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
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 usingstd::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