Improvement of my code

Question

I'm pretty new to C++ and want just to test how fast C++ can do following job:

Just create a vector with 100 Objectc of Object-Point (x,y-Coordinate) and move it to another vector. Repeat this k-times. (in this code it is 1000000-times - int Iterator).

Well since im very new to C++, do you see a better way to do it, or did i miss something?

Im running on Windows.

#include "Main.h"
#include "Point.h"
#include <iostream>
#include <vector>
#include <chrono>


int main() {
    auto start = std::chrono::high_resolution_clock::now();
    int Constant = 10;
    int Iterator = 1000000;

    std::vector<Point>* tour = new std::vector<Point>();
    std::vector<Point>* actions = new std::vector<Point>();

    for (int k=0; k<Iterator; k++) {

        for (int i=0; i<Constant; i++) {
            for (int j=0; j<Constant; j++) {
                Point *p = new Point((i * 10) + j,i + 1, j + 1);
                actions->push_back(*p);
            }
        }

        while(!actions->empty()) {
            tour->push_back(actions->at(0));
            actions->erase(actions->begin());
        }

        actions->clear();
        tour->clear();
    }

    auto finish = std::chrono::high_resolution_clock::now();
    std::cout << std::chrono::duration_cast<std::chrono::nanoseconds>(finish-start).count() << std::endl;
}

Is the program working? Does it do what you want it to do? Then all you need is a [code review](http://codereview.stackexchange.com/tour). — Some programmer dude, Oct 07 '16 at 10:18
If you want to improve working code you better post this question at [SE Code Review](http://codereview.stackexchange.com/). — πάντα ῥεῖ, Oct 07 '16 at 10:18
additional information of job: create a vector of 100 Objects with x,y-Coordinates + id and move it to another vector by adding the objects in incremental index order and delete the objects in the origin vector — kxell2001, Oct 07 '16 at 10:19
I suspect [*this technique*](http://stackoverflow.com/a/378024/23771) will show that *new Point* and the two *push_back* statements are responsible for essentially all of the time. — Mike Dunlavey, Oct 07 '16 at 12:34

Mr.C64 · Answer 1 · 2016-10-07T11:29:15.263

4

Consider allocating the vector instances on the stack, not on the heap, e.g:

std::vector<Point>* tour = new std::vector<Point>();
std::vector<Point>* actions = new std::vector<Point>();

just becomes:

// std::vector default constructor creates empty vectors.
std::vector<Point> tour;
std::vector<Point> actions;

Similarly, do not unnecessarily inefficiently allocate Points on the heap!

Point *p = new Point((i * 10) + j,i + 1, j + 1);
actions->push_back(*p);

Just do something much simpler and more efficient, like:

actions.push_back(Point{x, y, z});

Moreover, you can copy from one vector to the other just using overloaded operator=:

destVector = sourceVector;

As per your additional comment, if you want to move content from one vector to another, you can use std::move(), e.g.:

// Data moved from sourceVector to destVector.
// Leaves sourceVector empty.
destVector = std::move(sourceVector);

In addition, if you have compile-time constants, you can use constexpr:

constexpr int Constant = 10;
constexpr int Iterator = 1000000;

edited Oct 07 '16 at 11:29

answered Oct 07 '16 at 10:21

Mr.C64

41,637
14
86
162

Thnx for the fast response. This helped me alot. Yeah i forgot to mention that i just dont want a copy of a vector. Moreover i want to move just the objects to another vector step-by-step. This has something to do with another problem im dealing with. – kxell2001 Oct 07 '16 at 10:29
@kxell2001: You're welcome. Glad to be of help. – Mr.C64 Oct 07 '16 at 10:30
@kxell2001 - If you want to move all elements from one vector to another, you can do `destVector = std::move(sourceVector);`. This involves no copying. – Bo Persson Oct 07 '16 at 11:07
@kxell2001: You edited your initial comment. If you want to move (instead of a deep-copy), then there's `std::move()`, as already mentioned by @BoPersson. I edited my answer accordingly. – Mr.C64 Oct 07 '16 at 11:29
What sort of speed ups were seen using what is suggested here? – Paul Rooney Oct 07 '16 at 12:00

Paul Rooney · Answer 2 · 2016-10-08T06:20:09.477

1

I think the biggest slow down in your code is the fact you are erasing from the front of a vector. When you do this it moves every other element up one position. If you are doing this many times, you'll see that you are wasting a lot of processing power.

So instead just copy the vector.

while(!actions->empty()) {
    tour->push_back(actions->at(0));
    actions->erase(actions->begin());
}

becomes

tour = actions;

edited Oct 08 '16 at 06:20

answered Oct 07 '16 at 12:05

Paul Rooney

20,879
9
40
61

If you really want to move the elements individually from one vector to the other either do it in a loop without the erases. If you are insistent on the erases, reverse the actions vector first. As a result you will be erasing from the back of the vector which is a much more efficient operation. – Paul Rooney Oct 09 '16 at 21:38

Improvement of my code

2 Answers2