views:

427

answers:

6

I am working on a merge sort function. I got the sort down - I am trying to get my merge part finished. Assume that I am learning C++, have cursory knowledge of pointers, and don't understand all the rules of std::vector::iterator's (or std::vector's, for that matter).

Assume that num is the size of the original std::vector that have copied (std::copy) values from an array of size "int ar[num]." Assume that farray has the values of (0 to (num / 2)) and sarray has the values of ((num / 2) to num).

int num = original.size();
std::vector<int> final(num);

for (std::vector<int>::iterator it = farray.begin(); it != farray.end(); ++it) {
     for (std::vector<int>::iterator iter = sarray.begin(); iter != sarray.end(); ++iter) {
         if (*it > *iter) final.push_back(*it);
          else
               final.push_back(*iter);
     }
}

This code compiles and my latest stable build of Bloodshed Dev-C++ does not throw any warnings or errors. I don't know if this is valid, I still need to try and cout all the values of final. I just want to know if this is common, prone to errors, or just bad style. And, if so, how you would

+1  A: 

You can nest loops of any kind (for, while, do while) as long as you don't reuse the loop variables. If you would try that it would compile but may fail miserably during runtime. Although technically allowed to use the same name for nested loop variables in modern C and C++ it is confusing and should be avoided.

It's no more or less prone to errors than a single loop except for the already mentioned problem with the reuse of loop variables.

Read more about the limits of nested loops.

lothar
You can reuse loop variables in nested loops. It's not normally a good idea, but it doesn't automatically mean that it will crash at runtime if you're careful with what you do with them.
Peter
You mean like using int i = 0 to iterate over both loops? Would you increase it in the embedded loop or the outer loop?
Hooked
Failing at runtime does not necessarily mean crashing. It could simply calculate wrong results.
lothar
If you write 'int i' in each for loop, you get two separate variables because they're in different scope. In the inner loop you wouldn't be able to access the i from the outer loop.
Peter
No, it doesn't necessarily mean crashing, but my point still stands - just because you have two nested loops doesn't automatically mean that you'll get the wrong result. It's tricky but possible to get right.
Peter
I don't think it's wise to encourage a newby to use the same variable name in nested loops even if it's allowed and does the right thing if you know what you are doing. However it probably fails in the hands of not experienced programmers (and may even trip an experienced one).
lothar
As Peter said, if you use 'int i' in each loop, then they're two different variables - this makes for really confusing code. Also, if you use the same name, you can't access both at once (you just get the one with the most specific scope - so if you're in the inner loop, you don't see the 'int i' from the outer loop... and, if you've got nested loops, you probably want to use both at once, so there's not much point in doing it at all.
Smashery
@Smashery Exactly my point. That's why I recommend not doing it and naming each loop variable differently.
lothar
Well, I think it's a moot point - if they will be out of eachother's scope and lifetime, there would be no benefit in using the same name, correct? It's a devil's advocate's problem.
Hooked
A: 

Yes, you can do this. And yes, it is often prone to errors. In fact, writing loops is itself prone to errors, which is one argument to use the algorithms in the STL like for_each, copy and transform.

John Dibling
Haha, I am not that far ahead. I actually use std::copy in this program, to copy the values from the original array[num] to std::vector<int> original. Would you mind telling me what the former and latter are?
Hooked
I really wonder sometimes who these idiots that downvote responses like this are.
John Dibling
Hooked: for_each() is used to perform the same operation on each element in a range, like this: for_each( v.begin(), v.end(), doSomething() ); 'doSomething()' is a so-called functor, which will be executed against each value in 'v'.
John Dibling
std::transform() is very similar to copy(), with one big difference. Instead of just making a verbatim copy from one collection to another, it transforms the vector from one type to another using a conversion function you specify. for example suppose you have a vector of numbers and want to format up a vector of strings. you could write a functor which takes an int, uses stringstream << int to format up a string, and copy these values in to a vector<string>
John Dibling
A far more useful stl algorithm in this case would be merge (http://www.sgi.com/tech/stl/merge.html)
Eclipse
+2  A: 

Merge sort algorithm aside, nested for loop with iterator's is just as valid as nested for loops with two variables i and j.

Simeon Pilgrim
It was my understanding that iterators were my only option - can you use integers to iterate over std::vector v?
Hooked
Of course you can. Use the [] operator, just like an array, or the at() member function. Check your favorite STL documentation for details.
Rob Kennedy
iterators in this case will compile down to pointers, and just using the inder [] will behave the same, but, I was referring to a nature of nested loops in general.
Simeon Pilgrim
A: 

Yes, you can nest loops, or other statements, to pretty much whatever depth you want (within reason; there are limits, as mentioned in another answer, but they're way above what you should ever need).

Peter
+6  A: 

It's valid... but a for loop probably isn't what you want. When you use two for loops, your inner loop keeps going back to the start every time the outer loop loops. So if your vectors contain:

farray: 10 9 8 4 3
sarray: 7 6 4 3 1

Then your final array will contain something like:

10 10 10 10 10 9 9 9 9 9 8 8 8 8 8 7 6 4 4 4 7 6 4 3 3

because you are testing every single combination, and adding the larger one to the final list. A better solution might be to remember an iterator for each list, and just use one loop. Rather than looping over a list, just go through both of them together - if sarray has the larger number, then increment your sarray iterator, and compare that with the old farray iterator. Stop your loop when both sarray and farray are empty.

vector<int> fiter = farray.begin();
vector<int> siter = sarray.begin();
vector<int> final;

// Let's traverse both farray and sarray.
// We'll want to stop this loop once we've traversed both lists.
while (fiter != farray.end() && siter != sarray.end())
{
    if (fiter == farray.end())
    {
        // we must have gone right through farray - 
        // so use the value from sarray, and go to the next one
        final.push_back(*siter);
        siter++;
    }
    else if (siter == sarray.end())
    {
        // we must have gone right through sarray - 
        // so use the value from farray, and go to the next one
        final.push_back(*fiter);
        fiter++;
    }
    else if (*siter > *fiter)
    {
        // siter is the bigger of the two - add it to the final list, and
        // go to the next sarray entry
        final.push_back(*siter);
        siter++;
    }
    else // *fiter >= *siter
    {
        // fiter is the bigger of the two - add it to the final list, and
        // go to the next farray entry
        final.push_back(*fiter);
        fiter++;
    }
}

I haven't tested it - and if this is for homework, then please try to understand what I've done, go away and write it yourself, rather than copy+paste.

Smashery
Would you explain your implementation?
Hooked
I've given an example (untested) implementation - it just uses one while loop, and keeps going through until it's traversed both lists. If it's made it all the way through one list, it just adds those in the untraversed list. Otherwise, it compares the two items, and adds the bigger one, before incrementing the iterator within that list.
Smashery
In addition, 'final' is being created with 'num' entries of 0 to begin with, then the real entries are appended to that. I don't think that's desired behavior.
Fred Larson
OK, thanks for the example. I understand what you are doing here. Fred, what do you mean it's being created with 0 entries? In the rest of the mergeSort() code, I have an originalarray = { //ten values}, which I just use std::copy to put into std::vector<int> original.
Hooked
By the way, I am not even taking classes - I am learning this on my own time, with a few friends helping me. I wouldn't bother doing it if I didn't try to do it well.
Hooked
@Hooked: The line 'vector<int> final(num);' initializes the vector to have the number of entries specified in 'num'. These entries will be initialized with the default constructor, which for ints, initialize to 0. The subsequent push_back() calls will append entries after those 0 entries.
Fred Larson
If it sounds confusing to have a default constructor for a primitive type, it's because it's kind of a special case in C++. See this: http://stackoverflow.com/questions/563221/is-there-an-implicit-default-constructor-in-c/563229#563229
Fred Larson
Actually, I hadn't thought about it. I just wanted to specify the amount of memory to reserve - but if vectors resize automatically, it wouldn't matter. I was just used to using arrays where you do need to assign the amount of memory. I guess it would be described as a "benign bug" in this instance.
Hooked
Please assume that I'm guessing what a constructor is and "primitive type" has never been defined to me either. I'd be happy to hear what they are, though! :D
Hooked
Benign in the sense of getting the wrong answer. You result would be something like 0 0 0 0 0 0 0 0 0 0 10 9 8 7 6 4 4 3 3 1.
Fred Larson
I'll give you a hint: look into the difference between size and capacity for vectors. You're setting the size, you want to set the capacity. And I'm not going to be able to explain constructors in 300 chars or less. 8v) Maybe try this: http://www.parashift.com/c++-faq-lite/ctors.html
Fred Larson
Oh, I understand. I'm appending num 0s to final before it starts appending the values.
Hooked
Yes, that's it exactly.
Fred Larson
Gotcha, Fred. I actually have the faq-lite on my computer in some directory, I'll have to find it and read it over again.
Hooked
The vector constructor should not take the size as argument as it creates that many elements (plus the ones you push_back). You should either use the default constructor + resize or change the push_back's into 'final[pos++] =' with an index pos initialized with 0
David Rodríguez - dribeas
+1  A: 

Nesting for loops is a totally legit way to do things. For example, it's the classic "old school" way to traverse a 2D array - one loop goes down the y axis, and the other loop goes down the x axis.

Nowadays, with those kids and their for each loops and iterators and mapping functions there are arguably "better" ways to do it, (for some definition of better) but nesting loops works just fine. Using C++ or pointers doesn't change that.

Electrons_Ahoy