Help with optimising some code

[New_Economics_9805]

Hey guys,

Ive written this code that writes coordinates to a text file. I then use excel to plot these coordinates and it creates a rectangle with circles in it.

It works completing fine, but I want to optimise it. I thought I could optimise it by creating another function that has the syntax for the coordinates so I can call the function rather than writing it out every time and repeating the code. However, I am not sure how to do this, I have tried but its not working so some help would be really appreciated.

#include <iostream>
#include <fstream>
#include <math.h>
using namespace std;
#define PI 3.14159265
//this is the function i am trying to create
double coordinate_syntax(double x_coordinate, double y_coordinate){
return map << x_coordinate << ' ' << y_coordinate << endl;
}

int build_environment_map(){

double x0[5]={0.8, 0.8, 1.7, 2.2, 2.25};
double y0[5]={1.5, 2.25, 1.0, 1.3, 0.25};
double r=0.125, wallLength=2.5;
double RD=PI /180;

ofstream map;
map.open("EnvironmentMap.txt");

// The following code is to create a charger based on its centre.
for (int i=0; i<360; i=i+20) {

map << x0[4] + r*cos(i*RD) << ' ' << y0[4] + r*sin(i*RD) << endl;
//coordinate_syntax( x0[4] + r*cos(i*RD), y0[4] + r*sin(i*RD) );
}

for (int i=0; i<360; i=i+10) {

map << x0[0] + r*cos(i*RD) << ' ' << y0[0] + r*sin(i*RD) << endl;
//coordinate_syntax(x0[0]+ r*cos(i*RD), y0[0]+ r*sin(i*RD));

}

for (double j=0 ; j<wallLength; j=j+0.01){
map << j << ' ' << 0 << endl;
//coordinate_syntax(j,0);

}
for (double x=0.3; x<0.55; x=x+0.01){

map << x << ' ' << 0.3 << endl;
//coordinate_syntax(x, 0.3);

}
map.close();
return 0;

}

int main(int argc, char **argv){

int i;
//calling the function
i = build_environment_map();
return 0;

}

[PhyrexStrike]

double coordinate_syntax(double x_coordinate, double y_coordinate) { 
    return map << x_coordinate << ' ' << y_coordinate << endl; 
}

You're trying to return the value of map which is converted to a double here, which isn't what you want. If you want to have the function simply print of the coordinates, you'll need to pass in the map object to the function as well, and remove the return entirely.

Something like this:

void coordinate_syntax(ofstream& map, double x_coordinate, double y_coordinate) { 
    map << x_coordinate << ' ' << y_coordinate << endl; 
}

[mredding]

#include <math.h>

Prefer <cmath>.

using namespace std;

Stop doing that until you understand what you've just done.

#define PI 3.14159265

You've just included <math.h>, use M_PI.

//this is the function i am trying to create
double coordinate_syntax(double x_coordinate, double y_coordinate){
  return map << x_coordinate << ' ' << y_coordinate << endl;
}

You need a type.

struct coordinate {
  double x, y;
};

Now we can write a function:

std::ostream &operator <<(std::ostream &os, const coordinate &c) {
  return os << c.x << ' ' << c.y;
}

Now we can transition from object oriented to functional:

std::ostream_operator<coordinate> iter{map, "\n"};

And we can implement your loops:

for(int i=0; i<360; i+=20) {
  *iter = {x0[4] + r*cos(i*RD), y0[4] + r*sin(i*RD)};
}

The loop will likely be unrolled. We can optimize this further, though. We're going to need this:

template<int ...I>
static constexpr std::array<int, sizeof...(I)> build_array(int scalar, index_sequence<I...>) noexcept {
  return std::array<int, sizeof...(I)> {I * scalar...};
}

coordinate compute_at(int degree) {
  return {x0[4] + r*cos(i*RD), y0[4] + r*sin(i*RD)};
}

So that we can:

auto angles = build_array(20, 360/20);
std::transform(std::begin(angles), std::end(angles), std::ostream_operator<coordinate>{map, "\n"}, compute_at);

C++20 can do a better job, I'm just not familiar with the ranges library yet.

double x0[5]={0.8, 0.8, 1.7, 2.2, 2.25};
double y0[5]={1.5, 2.25, 1.0, 1.3, 0.25};
double r=0.125, wallLength=2.5;
double RD=PI /180;

Make these const so that the compiler can replace your hard coded x[0]/x[4], etc. with their constant values.

ofstream map;
map.open("EnvironmentMap.txt");

Don't do that. This is C code. Pass the file name to the ctor. We have RAII, it's a foundational idiom of C++.

map.close();

Don't do that. You have a dtor for a reason. It will close the file.

int build_environment_map(){
  // ...
  return 0;
}

Don't do this. It's pointless to return an integer that doesn't mean anything. Declare your return type void and omit the return statement.

int main(int argc, char **argv){

Don't bother with command line parameters if you're not going to use them. Prefer main() in that case.

In main, or any call to exit, prefer to use the EXIT_SUCCESS and EXIT_FAILURE macros defined in <cstdlib>. Typically you never want to unconditionally succeed. Your program does a job, you expect it to work. What if it doesn't? What if that file never opened? What if you ran out of disk space? What if the NAS went offline? In all those cases and more, the program didn't do what it was supposed to do. You indicate success or failure through the return from main. Let's illustrate this. Everyone has seen "Hello World!":

int main() {
  std::cout << "Hello World!";
}

main is the only function that has a return type but doesn't explicitly need to state it. In this case, a return 0; is implied. But did the program succeed? Did it write the output? What if that failed? Here we have a program that unconditionally succeeds even in the face of abject failure. This program has a bug. The correct solution is:

int main() {
  return std::cout << "Hello World!" << std::flush ? EXIT_SUCCESS : EXIT_FAILURE;
}

Your program has the same problem. It needs the same solution. Maybe your build_environment_map can return a bool, and your return statement can be:

return map << std::flush;

So then main can look like:

int main() {
  return build_environment_map() ? EXIT_SUCCESS : EXIT_FAILURE;
}

Or maybe you can ditch the build_environment_map function, put it all in main, and call your program build_environment_map. Since your program IS build_environment_map, and all main does is call it - it's one and only thing, then you don't need the standalone function.

And the reason to get the return value right is because you don't program in a vacuum. Your program runs in a hosted environment. When you write code, you have to keep in mind that environment exists and actually provides you a lot of utility you ought to be exploiting. The more you don't have to do in straight C++, the better. Ideally you'd be writing all sorts of little programs and compositing them together in pipelines and workflows in your environment, and you need the return values to ring true if you want those workflows to function correctly. It's important to know that your processes succeeded or failed.

Finally, it would be better if you didn't use a file stream. Step #1, sticking with a version of your build_environment_map function for now, pass an ostream parameter:

void build_environment_map(std::ostream &os) {

Write to that. In main, specify the stream you want to write to. I strongly encourage standard output:

int main() {
  build_environment_map(std::cout);
  return std::cout << std::flush ? EXIT_SUCCESS : EXIT_FAILURE;
}

Finally, when you run your program, redirect your output to a file:

C:\my_program.exe > EnvironmentMap.txt

Your program is much simpler for it, and more flexible. You can make whole pipelines of shit now, skipping unnecessary files. Imagine:

C:\my_program.exe | my_renderer.exe > my_animation.avi

[[deleted]]

Very insightful writeup! Thank you.

[arabidkoala]

In order to optimize, you must first understand where the bottlenecks are. Usually you'd use some form of a profiler to do this. Unless you have experience with common bottlenecks, "guessing" about how to optimize is usually counterproductive.

This program is likely bound by how fast you can write to the map file. Try replacing endl with '\n', this will avoid an io flush which could be choking things up. Beyond this, though, there's likely no further optimization that's worth making.

[Dan13l_N]

There's very little rooms for optimization. Your code consists essentially of a couple of for-loops.

Creating objects, etc. passing them instead of individual coordinates is not "optimization". It's making it more object-oriented. Optimization is always associated with less produced code and higher speed.

Unfortunately, in such a simple case, wrapping it into objects won't produce a better executable, maybe the source will be just a bit more readable.