patterncppMinor
Dynamic array container
Viewed 0 times
arraycontainerdynamic
Problem
This is primarily a container for quicksort and mergesort:
#include "c_arclib.cpp"
template class dynamic_array
{
private:
T* array;
T* scratch;
public:
int size;
dynamic_array(int sizein)
{
size=sizein;
array = new T[size]();
}
void print_array()
{
for (int i = 0; i array[r]))
{
scratch[i] = array[l];
l++;
}
else
{
scratch[i] = array[r];
r++;
}
}
for(i = left; i pivot)r--;
if (l d1(10);
cout << d1.size;
d1.print_array();
d1.rand_to_array();
d1.print_array();
d1.merge_sort();
d1.print_array();
}Solution
My first comment is its named badly.
dynamic_array implies that I can use [] operator on it and get a value out.
You have owned RAW pointers in your structure.
First this means you need to look up RAII to make sure these members are correctly deleted.
Second you you need to look up the rule of three (or 5 in C++11) to make sure they are copied correctly.
You have owned RAW pointers in your structure. This means you need to correctly manage the object as a resource. This means constructions/destruction/copy (creation and assignment) need to be taken care of correctly.
Either do this manually or use a standard container that will do it for you. I suggest a standard container.
If you are going to write print_array at least write it so that it can use alternative stream (not just std::cout). Then write the output operator.
Also note that a method that access data but does not modify the state of the object should be marked const. So the signature should be:
Are the following members really part of the array?
OK. Lets assume they are for now.
Then
scratch will Never be NULL.
I think merge (in merge_recurse) is easier to write than you are making it:
You should only call srand() once in an application:
By putting srand() inside the structure you are opening it up to be called multiple times. Call it once just after main() then don't call it again.
When you can use the standard tools:
Can be replaced with:
I am relatively sure these two are wrong:
They should be:
dynamic_array implies that I can use [] operator on it and get a value out.
You have owned RAW pointers in your structure.
private:
T* array;
T* scratch;First this means you need to look up RAII to make sure these members are correctly deleted.
Second you you need to look up the rule of three (or 5 in C++11) to make sure they are copied correctly.
You have owned RAW pointers in your structure. This means you need to correctly manage the object as a resource. This means constructions/destruction/copy (creation and assignment) need to be taken care of correctly.
Either do this manually or use a standard container that will do it for you. I suggest a standard container.
void print_array()
{
for (int i = 0; i < size; i++) cout << array[i] << endl;
}If you are going to write print_array at least write it so that it can use alternative stream (not just std::cout). Then write the output operator.
std::ostream& operator<<(std::ostream& stream, dynamic_array const& data)
{
data.print_array(stream); // After you fix print_array
return stream;
}Also note that a method that access data but does not modify the state of the object should be marked const. So the signature should be:
void print_array() constAre the following members really part of the array?
void merge_recurse(int left, int right)
int merge_sort()
void quick_recurse(int left, int right)OK. Lets assume they are for now.
Then
void merge_recurse(int left, int right) should be a private member. There should be no reason to call this from externally.scratch = new T[size]();
if(scratch != NULL)scratch will Never be NULL.
I think merge (in merge_recurse) is easier to write than you are making it:
int index = 0;
int l = left;
int r = midpoint;
while((l array[r]))
? array[l++]
: array[r++];
}
// One of the two ranges is empty.
// copy the other into the destination.
while(l < midpoint)
{
scratch[index++] = array[l++];
}
while(r < right)
{
scratch[index++] = array[r++];
}You should only call srand() once in an application:
void rand_to_array()
{
srand(time(NULL));By putting srand() inside the structure you are opening it up to be called multiple times. Call it once just after main() then don't call it again.
When you can use the standard tools:
tmp = array[l];
array[l] = array[r];
array[r] = tmp;Can be replaced with:
std::swap(array[l], array[r]);I am relatively sure these two are wrong:
while (l <= r)
if (l <= r)They should be:
while (l < r)
if (l < r)Code Snippets
private:
T* array;
T* scratch;void print_array()
{
for (int i = 0; i < size; i++) cout << array[i] << endl;
}std::ostream& operator<<(std::ostream& stream, dynamic_array const& data)
{
data.print_array(stream); // After you fix print_array
return stream;
}void merge_recurse(int left, int right)
int merge_sort()
void quick_recurse(int left, int right)scratch = new T[size]();
if(scratch != NULL)Context
StackExchange Code Review Q#5745, answer score: 3
Revisions (0)
No revisions yet.