Searching for efficient code

[dekeenfrance]

Hello everyone,

In order to do some simulations, I create a class containing vectors, then a vector of objects of this class. Once it's done, I need to check vectors in each object whether there is negative elements. If it is the case, I have to increase each negative element of 0.5 in a round, till there is no negative element anymore.

I have written following code. However, it cannot compile. There are some problems with the lambda expression, maybe also with other fractions.

Could someone please tell me where is the mistake, or how can I code in a more efficient way to make the execution fast?

Thanks in advance.

int main()
{
	vector<CTest*>::iterator it;
	for(int i=0; i<4; i++)
	{
		it=find_if(myVec.begin(), myVec.end(), [](CTest *v, int i)->bool{return (*v).length[i]<0;});
		while(*it)
			*it+=0.5;
	}
	return 0;
};

[Catfish3]

1) You try to pass variable i to the lambda instead of capturing it.
it=find_if(myVec.begin(), myVec.end(), [&i](CTest *v)->bool{return (*v).length[i]<0;});

2) Memory leak. In C++, usually every new must have its corresponding delete.
myVec.push_back(new CTest(al[0]));
If you want to stick to dynamic memory allocation, even if you don't need to in my opinion, look into smart pointers:
http://en.cppreference.com/w/cpp/memory
Then you can "forget" about the delete.

3) What does *it+=0.5; mean?
*it returns a pointer to a CTest object.

[dekeenfrance]

Thank you Catfish3 for your help.

I wanted to increase each negative element of each vector 0.5 till there is no negative element. There was a code mistake.

To be brief, 1. check whether there exists negative element. 2. If true, increment all negative element 0.5. 3. recheck for negative element. 4. If true,... I tried to use do... while structure, it doesn't fit; neither the while structure.

Any idea to write it efficiently, please?

here is the corrected code.

	vector<CTest*>::iterator it;
	for(int i=0; i<4; i++)
	{
		it=find_if(myVec.begin(), myVec.end(), [&i](CTest *v)->bool{return (*v).length[i]<0;});
		while(*it)
			(*it).length[i]+=0.5;
	}

Thank you also for the recall of memory leak. I will check on smart pointers.

[Catfish3]

Any idea to write it efficiently, please?

If your compiler supports C++11 range-based for() loops, you can simplify your code to (untested):

for (CTest *&ct: myVec)
{
    if (ct->length[0] < 0) ct->length[0] += 0.5;
    if (ct->length[1] < 0) ct->length[1] += 0.5;
    if (ct->length[2] < 0) ct->length[2] += 0.5;
    if (ct->length[3] < 0) ct->length[3] += 0.5;
}

Edit:

Thank you also for the recall of memory leak. I will check on smart pointers.

Personally, I would suggest you to keep things simple and not use dynamic memory allocation unless you need it, and not just because you want it.

[dekeenfrance]

Thanks Catfish3 for your quick reply.

I use VS2010 so it should support c++11. However, I would like to use some traditional for loop like for(int i=0; i<4; i++) then use some "automatic" control structure to check negative element of vectors and adjust them, and recheck, and readjust, and ....

Actually, here is just a demonstration code. The vector "myVec" in reality contains thousands of objects, each objects contains 40 vectors like length, and the whole iteration round int i runs from 0 to 1000.

That's why I cannot write all the case one by one with if.

[Catfish3]

They didn't add range-based for() loops to VS2010, only VS2012.

How about (untested, again):

std::for_each(myVec.begin(), myVec.end(), [](CTest *&ct) {
    std::for_each(ct->length.begin(), ct->length.end(), [](double &d) {
        if (d < 0)
            d += 0.5;
    });
});

[dekeenfrance]

Well, it seems in your code we check the vector of length from begin to end. But actually I need to check each length[0] of each object at one time. Then move to length[1] of each object...

I have just modified my previous code following a traditional way:

	for(int i=0; i<3; i++)
	{
		vector<CTest*>::iterator it;

		it=find_if(myVec.begin(), myVec.end(), [&i](CTest *v)->bool{return (*v).length[i]<0;});
		while(it!=myVec.end())
		{
			auto iter(myVec.begin());
			while(iter!=myVec.end())
				(*(iter++))->length[i]+=0.5;
			it=find_if(myVec.begin(), myVec.end(), [&i](CTest *v)->bool{return (*v).length[i]<0;});
		}
	}

1. check whether there is a negative element. 2. if yes, increment the element of all vectors by 0.5. 3. recheck for negative element... till there is no negative element, turn to next level of i.

Is this code correct? Even if it's correct, I feel it too complicated, not efficient.

[Catfish3]

I do not understand what you need to do.

1) So you have a vector of objects (V_o), all objects containing a vector of doubles (V_d).
2) You check each element in the V_d of each object in V_o to see if it's negative.
3) If it is you keep position i, and increase all elements at V_d[i] of all objects in V_o by 0.5?

[dekeenfrance]

Yes, exactly.

for 3): And then recheck all Vd[i], whether there is negative. if yes, increase by 0.5 again. And recheck... till there is no negative element among all Vd[i].

[Catfish3]

So then is this closer to your goal?

std::for_each(myVec.begin(), myVec.end(), [](CTest *&ct) {
    for (std::vector<double>::size_type i=0; i < ct->length.size(); ++i)
        if (ct->length[i] < 0)
            std::for_each(myVec.begin(), myVec.end(), [i](CTest *&ct) {
                ct->length[i] += 0.5;
            });
});

Edit: old code garbage.

[dekeenfrance]

Yes. I think your code get the same results as mine. but yours is shorter.

Thanks a lot!