4
\$\begingroup\$

This is code I wrote for calculating bezier curves as quickly and RAM efficiently as possible.

I would like to know if there are faster ways to optimize anything, because I am very new to C++ and efficiency methods.

Lastly, if there is a variable that could be more explicitly named, also let me know.

#include <vector>

double* bezier_interp (double* points, double* temp_space, int temp_space_size, int points_total, double t_step_t) {
    memcpy(temp_space, points, temp_space_size);
    for (int pointsi = points_total; pointsi > 1; --pointsi){
        for (int pointsj = 1; pointsj < pointsi; pointsj++) {
            points[pointsj-1] -= (points[pointsj-1] - points[pointsj]) * t_step_t;
        }
    }
    return points;
}

double* bezier_t (double* numbers, int points_total, int definition) {
    double *result = new double[definition];

    double t_step = 1 / ((double) definition - 1);
    double t_step_t;
    double* temp_space = new double[points_total];
    int temp_space_size = sizeof(*temp_space) * points_total;
    for (int t = 0; t < definition; t++) {
        t_step_t = t * t_step;
        result[t] = *bezier_interp(numbers, temp_space, temp_space_size, points_total, t_step_t);
    }
    delete[] temp_space;
    return result;
}

#include<iostream>

int main() {
    double numbers_x[] = {
        0, 0, 4, 4
    };
    double numbers_y[] = {
        0, 4, 4, 0
    };
    int definition = 8;
    double* xs = bezier_t(numbers_x, 4, definition);
    double* ys = bezier_t(numbers_y, 4, definition);
    for (int dix = 0; dix < definition; dix++){ std::cout << xs[dix] << "\t"; }
    std::cout << "\n";
    for (int diy = 0; diy < definition; diy++){ std::cout << ys[diy] << "\t"; }
    std::cin.get();
    return 0;
}
\$\endgroup\$
2
\$\begingroup\$

Memory management

The bezier_t function allocates memory for result variable:

    double *result = new double[definition];

The function returns result and the memory is never freed. Be careful with memory management. The cleanest solution is to allocate and free memory in the same method. But this function obviously cannot do that, because it needs to return the result right?

To overcome this, you can allocate result before calling bezier_t, pass it to bezier_t as a parameter, and free sometime after bezier_t returns.

Related and similar to this is the bezier_interp function. It takes the points array as parameter, modifies its contents, and returns it. It's not wrong to return it, but it's a bit confusing, because whenever I see something that looks like a pointer, I have to check where it comes from, where is it allocated and freed.

You could change bezier_interp to return void, and write the loop body in bezier_t like this:

bezier_interp(numbers, temp_space, temp_space_size, points_total, t_step_t);
result[t] = numbers[0];

Limit variables to the smallest scope necessary

There's no need to declare the t_step_t variable before the loop:

double t_step_t;
for (int t = 0; t < definition; t++) {
    t_step_t = t * t_step;
    // ...

It's better to declare the variable inside the loop:

for (int t = 0; t < definition; t++) {
    double t_step_t = t * t_step;
    // ...

This way it cannot be accidentally misused outside the loop. Sure, in this program such accidents seem unlikely for now, but it's a good habit to get used to.

And perhaps t_t_step would be somewhat better name than t_step_t.

Simplify

This expression can be written simpler:

double t_step = 1 / ((double) definition - 1);

Like this:

double t_step = 1. / (definition - 1);

Misc

I'm totally with @vnp: pointi and pointj are really hard to read, simple i and j would be better.

\$\endgroup\$
  • \$\begingroup\$ Thank you janos and @vnp, this is my first code review post, should I resubmit my code, or update this question by appending the newest version here? How does further revision take place? \$\endgroup\$ – Cole Lawrence Jan 30 '15 at 23:44
  • 1
    \$\begingroup\$ Don't edit the code in the question, as that would invalidate the answers. Create a new question with your updated code, and include a link to this question, for reference. \$\endgroup\$ – Stop ongoing harm to Monica Jan 31 '15 at 0:12
2
\$\begingroup\$
  • #include <vector> is pointless. There is no vectors in the code.

  • Unless I am missing something obvious, temp_space plays no role in the calculations.

  • pointsi and pointsj do not sound right (too much of visual noise makes it hard to tell them apart). It is one of the rear cases where I'd go for plain i and j.

  • t_step_t is confusing, and strictly speaking unnecessary. You may calculate it at the point of invocation as

    result[t] = *besier_interp(..., t * t_step);
    
\$\endgroup\$
  • \$\begingroup\$ No you are right, I wasn't using my temp_space for my calculations as I intended, because I wanted to be using the temp_space to store my evaluations in between pointsj and pointsi for loops. \$\endgroup\$ – Cole Lawrence Jan 30 '15 at 23:46
  • \$\begingroup\$ I had vector included for the memcpy function that I was using. I'll change it to <cstring> or something. \$\endgroup\$ – Cole Lawrence Jan 30 '15 at 23:49

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.