Quickest way to iterate in a C++ vector

Question

I was looking for some tips regarding vector iteration. I am trying to fulfill a challenge, in which you have to implement a function (SumOfTwo) that takes two vectors and a sum as arguments and has to check if there is any pair of numbers in the two vectors that if added can give the sum that is passed as an argument.

My function works fine, except that it doesn't cover the time limitation which is 500ms. i feel that there is a better way to iterate through two vectors than two for loops, but I feel very stuck...

bool sumOfTwo(std::vector<int> a, std::vector<int> b, int v) {
    if((a.empty()) || b.empty()){
        return false;
    }
    for (std::vector<int>::iterator it1 = a.begin(); it1<=a.end(); ++it1)     {
            for (std::vector<int>::iterator it2 = b.begin(); it2<=b.end(); ++it2) {
                if ((*it1 + *it2) == v){
                    return true;
                }
            }
    }
    return false;
}

Taking the vectors via reference (rather than by value which forces a copy) might help increase performance when calling the function — UnholySheep, May 15 '17 at 20:18
_"My function works fine"_ questions to improve working code better go to [SE Code Review](https://codereview.stackexchange.com/). — πάντα ῥεῖ, May 15 '17 at 20:18
^ you want `it1 != a.begin()`, not <=. < is correct for indices. — jaggedSpire, May 15 '17 at 20:20
I'm voting to close this question as off-topic because it belongs to [Code Review](https://codereview.stackexchange.com/). — gsamaras, May 15 '17 at 20:22
This is a classic XY problem. You've jumped to the conclusion that you need to make your nested `for` loops faster. Nope! — Lightness Races in Orbit, May 15 '17 at 20:48
if you have some reasonable guarantee on the max/min values of the numbers in the arrays, there is an O(n) radix sort solution, but uses space dependent on the input range. It's basically the same as the hash lookup, except without a hash function. — Kenny Ostrom, May 17 '17 at 13:32

Curious · Answer 1 · 2017-05-16T00:22:21.480

5

Regarding just style tips, change your code to the following (for efficiency and neatness)

bool sumOfTwo(const std::vector<int>& a, const std::vector<int>& b, int v) {
    for (auto i1 : a)     {
            for (auto i2 : b) {
                if ((i1 + i2) == v){
                    return true;
                }
            }
    }
    return false;
}

Consider using an std::unordered_set and cross referencing that to find whether a sum is possible for maximal efficiency. Remember that lookups in an std::unordered_set are O(1)

edited May 16 '17 at 00:22

answered May 15 '17 at 20:21

Curious

20,870
8
61
146

2

@downvoter, please comment on what I can do to improve this answer? – Curious May 15 '17 at 20:23
1

Since it's a competition/learning site question, I personally wouldn't just tell the answer. – Kenny Ostrom May 15 '17 at 20:24
@KennyOstrom thats why I didnt write the code to solve it, its really easy to find the answer to this via google. 2 sum is a very common problem. Do you still think the answer is inappropriate? Anyway, I removed the explanation – Curious May 15 '17 at 20:25
If it were me, I wouldn't say more than consider using unordered_map, but in comments I said basically "use a different approach, do you want a hint?" – Kenny Ostrom May 15 '17 at 20:27
I'm good with that, if your intent is to correct the basic iterator error in the OP. But also point out what the error was, in addition to showing the new (less error prone) way. Of course the other answer still ruins this person's chance to learn. – Kenny Ostrom May 15 '17 at 20:30
@erip I can see KennyOstrom's argument, answers to homework and challenge problems should not be given out on SO immediately. It can be unfair to others – Curious May 15 '17 at 20:32
2

@Curious I'm not a TA. If a grown person asks a question on SO, I'm going to answer it if it is within the scope of the site and the poster has given his/her good-faith effort. – erip May 15 '17 at 20:33
Right, if they wanted someone to just tell them the final answer, they would have already googled it. But they actually just wanted to know what was wrong with the answer they had so far. – Kenny Ostrom May 15 '17 at 20:33
@KennyOstrom: How the hell does teaching somebody "ruin their chance to learn"? – Lightness Races in Orbit May 15 '17 at 20:49

score 3 · Accepted Answer · edited Jun 20 '20 at 09:12

3

Why is the algorithm slow?

Currently you're iterating over two vectors, but you're doing nested iteration. Let's call the lengths of the vectors m and n (arbitrarily). Your current algorithm is O(n*m), which is quite slow for large vectors.

If m = n = 1000, in the worst-case (no two elements sum to v), you're doing millions of operations -- whew!

How can I improve the performance?

I can think of one immense speedup: given a vector, a, a vector b, and a desired sum v, you could create a set of numbers from the element-wise difference of v and a and check if any of the values in b are in the set.

The pseudocode is:

if a.empty or b.empty:
    return false

set = emptySet

for e in a:
    set.add(v - e)

for e in b:
    if e in set:
        return true

return false

Lookups in an unordered set should be O(1), so this is now an O(max(m, n)) algorithm where m is length of a and n is length of b.

If m = n = 1000, in the worst-case you're now doing thousands of operations -- much better, right?

Implementing this in C++ is left to the reader. :)

Hint: passing by reference is a good idea for vectors.

edited Jun 20 '20 at 09:12

Community

1
1

answered May 15 '17 at 20:25

erip

16,374
11
66
121

wasn't me, but still -- same as other answer, same as in comments to OP – Kenny Ostrom May 15 '17 at 20:29
really O(1)? Ie no matter now large the set it always takes the same time. Surely its O(log(n)) – pm100 May 15 '17 at 20:39
1

@pm100 Average case is O(1) because of hashing. http://www.cplusplus.com/reference/unordered_set/unordered_set/find/. Worst case is linear in size of set, true. And ordered sets will be O(log n) because they are implemented as self-balancing trees. – erip May 15 '17 at 20:40
@erip I think there is some confusion because c++ has a `set` which is a binary search tree. – juanchopanza May 15 '17 at 20:59
@juanchopanza Indeed that's from where the confusion indubitably stems (heh, couldn't help it). Alas, in my answer I reference _unordered sets_ explicitly. :) – erip May 15 '17 at 21:00

score 2 · Answer 3 · answered May 15 '17 at 21:08

2

Thank you all for your great help! The way i solved the time restriction is:

bool sumOfTwo(std::vector<int>& a, std::vector<int>& b, int v) {
   if((a.empty()) || b.empty()){
       return false;
   }
   std::unordered_set<int> s;
   for (const auto i : a) {
       s.insert(v-i);
   }
   for (const auto i : b) {
      auto search = s.find(i);
      if (search != s.end()) {
          return true;
      }
   }
   return false;
}

answered May 15 '17 at 21:08

Cooli

169
3
14

1

You might also like [`std::any_of`](http://www.cplusplus.com/reference/algorithm/any_of/) where your pred might be `s.find(i) != s.end()`. – erip May 15 '17 at 21:51

Quickest way to iterate in a C++ vector

3 Answers3

Why is the algorithm slow?

How can I improve the performance?