Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

To mitigate the verbosity you can use boost::format see Which C I/O library should be used in C++ code?

To mitigate the verbosity you can use boost::format see Which C I/O library should be used in C++ code?

added 2455 characters in body
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341
template<typename I>
void merge(I p, I q, I r)
{
 int leftSize = std::distance(p, q);
 int rightSize = std::distance(q, r);
 int L[leftSize]; // Technically not legal but most compilers support it.
 int R[rightSize]; // Normally use vectors here. But I am using the same
 // technique as shown by the OP
 std::move(p, q, L);
 std::move(q, r, R);
 int left = 0;
 int right = 0;
 I d = p;
 while(left < leftSize && right < rightSize)
 {
 (*d) = std::move((L[left] <= R[right])
 ? L[left++]
 : R[right++]);
 ++d;
 }
 // Note only one of these copies will actually do anything.
 std::move(pL + left, qL + leftSize, d);
 std::move(qR + right, rR + rightSize, d);
}

Use C++ fstream

FILE * resultsfile;
resultsfile=fopen("results-comparison_sort-noBS.dat","w");
for(int j=0;j< npoints;j++) fprintf(resultsfile, "%5e \t %10.2f \t %10.2f \t %10.2f \t %10.2f \t %10.2f \n",nplist[j], bubble_timelist[j], insertion_timelist[j], merge_timelist[j], heap_timelist[j], quick_timelist[j]);
fclose(resultsfile);

Prefer to use C++ fstream object (it is excepion safe unlike fopen/fclose).

Now admittedly the C++ stream operators are much much much more verbose then the C code for printing. But the main advantage is that they are TYPE SAFE so you have a much less chance of getting it wrong (though modern compilers actually check this in C now).

To mitigate the verbosity you can use boost::format see Which C I/O library should be used in C++ code?

A basic translation into C++

std::ofstream resultsfile("results-comparison_sort-noBS.dat");
for(int j=0;j< npoints;j++) {
 resultsfile << boost::format("%5e \t %10.2f \t %10.2f \t %10.2f \t %10.2f \t %10.2f \n")
 % nplist[j] 
 % bubble_timelist[j]
 % insertion_timelist[j]
 % merge_timelist[j]
 % heap_timelist[j]
 % quick_timelist[j]);
}

But if you had organized your times into a structure:

 struct TimePoints
 {
 int nplist;
 int bubble_timelist
 int insertion_timelist;
 int merge_timelist;
 int heap_timelist;
 int quick_timelist;
 friend std::ostream& operator<<(std::ostream& str, TimePoints const& d)
 {
 return str << boost::format("%5e \t %10.2f \t %10.2f \t %10.2f \t %10.2f \t %10.2f \n")
 % d.nplist
 % d.bubble_timelist
 % d.insertion_timelist
 % d.merge_timelist
 % d.heap_timelist
 % d.quick_timelist;
 }
 }
 // Then stored your times in a vector:
 std::vector<TimePoints> times;
 // Now you could print easily in a couple of ways
 std::copy(std::begin(times), std::end(times),
 std::ostream_iterator<TimePoints>(std::cout, "\n")
 );
 // or
 for(TimePoints const& tp: times) {
 std::cout << tp << "\n";
 }
template<typename I>
void merge(I p, I q, I r)
{
 int leftSize = std::distance(p, q);
 int rightSize = std::distance(q, r);
 int L[leftSize]; // Technically not legal but most compilers support it.
 int R[rightSize]; // Normally use vectors here. But I am using the same
 // technique as shown by the OP
 std::move(p, q, L);
 std::move(q, r, R);
 int left = 0;
 int right = 0;
 I d = p;
 while(left < leftSize && right < rightSize)
 {
 (*d) = std::move((L[left] <= R[right])
 ? L[left++]
 : R[right++]);
 ++d;
 }
 // Note only one of these copies will actually do anything.
 std::move(p + left, q, d);
 std::move(q + right, r, d);
}
template<typename I>
void merge(I p, I q, I r)
{
 int leftSize = std::distance(p, q);
 int rightSize = std::distance(q, r);
 int L[leftSize]; // Technically not legal but most compilers support it.
 int R[rightSize]; // Normally use vectors here. But I am using the same
 // technique as shown by the OP
 std::move(p, q, L);
 std::move(q, r, R);
 int left = 0;
 int right = 0;
 I d = p;
 while(left < leftSize && right < rightSize)
 {
 (*d) = std::move((L[left] <= R[right])
 ? L[left++]
 : R[right++]);
 ++d;
 }
 // Note only one of these copies will actually do anything.
 std::move(L + left, L + leftSize, d);
 std::move(R + right, R + rightSize, d);
}

Use C++ fstream

FILE * resultsfile;
resultsfile=fopen("results-comparison_sort-noBS.dat","w");
for(int j=0;j< npoints;j++) fprintf(resultsfile, "%5e \t %10.2f \t %10.2f \t %10.2f \t %10.2f \t %10.2f \n",nplist[j], bubble_timelist[j], insertion_timelist[j], merge_timelist[j], heap_timelist[j], quick_timelist[j]);
fclose(resultsfile);

Prefer to use C++ fstream object (it is excepion safe unlike fopen/fclose).

Now admittedly the C++ stream operators are much much much more verbose then the C code for printing. But the main advantage is that they are TYPE SAFE so you have a much less chance of getting it wrong (though modern compilers actually check this in C now).

To mitigate the verbosity you can use boost::format see Which C I/O library should be used in C++ code?

A basic translation into C++

std::ofstream resultsfile("results-comparison_sort-noBS.dat");
for(int j=0;j< npoints;j++) {
 resultsfile << boost::format("%5e \t %10.2f \t %10.2f \t %10.2f \t %10.2f \t %10.2f \n")
 % nplist[j] 
 % bubble_timelist[j]
 % insertion_timelist[j]
 % merge_timelist[j]
 % heap_timelist[j]
 % quick_timelist[j]);
}

But if you had organized your times into a structure:

 struct TimePoints
 {
 int nplist;
 int bubble_timelist
 int insertion_timelist;
 int merge_timelist;
 int heap_timelist;
 int quick_timelist;
 friend std::ostream& operator<<(std::ostream& str, TimePoints const& d)
 {
 return str << boost::format("%5e \t %10.2f \t %10.2f \t %10.2f \t %10.2f \t %10.2f \n")
 % d.nplist
 % d.bubble_timelist
 % d.insertion_timelist
 % d.merge_timelist
 % d.heap_timelist
 % d.quick_timelist;
 }
 }
 // Then stored your times in a vector:
 std::vector<TimePoints> times;
 // Now you could print easily in a couple of ways
 std::copy(std::begin(times), std::end(times),
 std::ostream_iterator<TimePoints>(std::cout, "\n")
 );
 // or
 for(TimePoints const& tp: times) {
 std::cout << tp << "\n";
 }
added 754 characters in body
Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341

I would have written merge like this:

template<typename I>
void merge(I p, I q, I r)
{
 int leftSize = std::distance(p, q);
 int rightSize = std::distance(q, r);
 int L[leftSize]; // Technically not legal but most compilers support it.
 int R[rightSize]; // Normally use vectors here. But I am using the same
 // technique as shown by the OP
 std::move(p, q, L);
 std::move(q, r, R);
 int left = 0;
 int right = 0;
 I d = p;
 while(left < leftSize && right < rightSize)
 {
 (*d) = std::move((L[left] <= R[right])
 ? L[left++]
 : R[right++]);
 ++d;
 }
 // Note only one of these copies will actually do anything.
 std::move(p + left, q, d);
 std::move(q + right, r, d);
}

##Avoid using dynamic allocation

##Avoid using dynamic allocation

I would have written merge like this:

template<typename I>
void merge(I p, I q, I r)
{
 int leftSize = std::distance(p, q);
 int rightSize = std::distance(q, r);
 int L[leftSize]; // Technically not legal but most compilers support it.
 int R[rightSize]; // Normally use vectors here. But I am using the same
 // technique as shown by the OP
 std::move(p, q, L);
 std::move(q, r, R);
 int left = 0;
 int right = 0;
 I d = p;
 while(left < leftSize && right < rightSize)
 {
 (*d) = std::move((L[left] <= R[right])
 ? L[left++]
 : R[right++]);
 ++d;
 }
 // Note only one of these copies will actually do anything.
 std::move(p + left, q, d);
 std::move(q + right, r, d);
}

##Avoid using dynamic allocation

Source Link
Loki Astari
  • 97.7k
  • 5
  • 126
  • 341
Loading
lang-cpp

AltStyle によって変換されたページ (->オリジナル) /