patterncppMinor
Finding two movies whose lengths add up to a given length
Viewed 0 times
whosemovieslengthtwolengthsfindinggivenadd
Problem
Here's some code which, given a list of \$N\$ movie lengths, checks if there are two movies that equal the exact length of a flight. This solution runs in \$\mathcal{O}(N)\$ time since I used a hash table.
In the second
#include
#include
#include
using namespace std;
int main() {
const int LENGTH = 170;
vector movies = {60, 120, 50, 150, 30, 45};
unordered_map secondMovies;
for (int movie: movies)
secondMovies[movie] = true;
for (int firstMovie: movies)
if (secondMovies[LENGTH-firstMovie]) {
cout << firstMovie << " and " << LENGTH-firstMovie;
return 0;
}
cout << "No movies found.";
return 0;
}In the second
for loop, I immediately return from main if two valid movies are found. Is this considered bad practice?Solution
Returning from the body of the loop isn't necessary a problem in itself. Nonetheless, your code does have a fairly significant problem.
When you search for a second movie of a particular length, you use the subscript operator. The problem arises from the fact that when this doesn't find an existing entry for a particular length, it inserts a new entry for that length. The value associated with the key will be value-initialized, will will give
That means the code will work, but because you're potentially inserting a lot of unnecessary items into your map, you could waste a lot of time. As long as you're only dealing with half a dozen movies, it's probably irrelevant, but if you deal with a lot it could get ugly.
In fact, there doesn't seem to be any reason to use a map here at all. It looks like an
There is something else here that could probably be viewed as a bug: if you have a movie whose length is exactly half the total length of the flight, it appears that your code will suggest watching the same movie twice in succession, which probably isn't something anybody would actually want.
When you search for a second movie of a particular length, you use the subscript operator. The problem arises from the fact that when this doesn't find an existing entry for a particular length, it inserts a new entry for that length. The value associated with the key will be value-initialized, will will give
false in this case.That means the code will work, but because you're potentially inserting a lot of unnecessary items into your map, you could waste a lot of time. As long as you're only dealing with half a dozen movies, it's probably irrelevant, but if you deal with a lot it could get ugly.
In fact, there doesn't seem to be any reason to use a map here at all. It looks like an
unordered_set will work perfectly fine for the task at hand.There is something else here that could probably be viewed as a bug: if you have a movie whose length is exactly half the total length of the flight, it appears that your code will suggest watching the same movie twice in succession, which probably isn't something anybody would actually want.
Context
StackExchange Code Review Q#138302, answer score: 2
Revisions (0)
No revisions yet.