Basic Algorithm
At least if I understand the intent correctly, you simply want a count of the unique input characters that occurred at least twice.
In that case, I think I'd do something like this:
int count_dupes(std::vector<int> const &inputs) {
std::map<int, int> counts;
for (auto i : inputs)
++counts[i];
return std::count_if(counts.begin(), counts.end(),
[](auto const &p) { return p.second >= 2; });
}
I'd also consider using an array instead of a map, as outlined in an answer to an earlier question: https://codereview.stackexchange.com/a/208502/489 --but this can depend on the range of values you're dealing with. With a 16-bit int, it's no problem at all on most machines. With a 32-bit int
(and no other constraints on values) it's still possible on many machines, but probably impractical. For arbitrary 64-bit int, an array won't be practical.
Parameter Passing
Right now, you're passing the input by value. This means when you call the function with some vector, a copy of the original vector will normally be made and passed to the function. As a general rule, something like a vector that's potentially large and slow to copy should be passed by reference to const, as shown in the code above.
Logical Comparisons
Comparing a Boolean value to true
or false
is generally a poor idea. if (x==true)
is equivalent to if (x)
and if (x == false)
is equivalent to if (!x)
. Normally, if it's Boolean in nature, a variable should be given a name that reflects that nature, and should be used directly rather than being compared to true
or false
. For example, s.insert(n).second == false
wold be better written as: if (!s.insert(n).second)
.
Some people (understandably, I guess) prefer to use the written form: if if (not s.insert(n).second)
. I've written C and C++ long enough that I have no difficulty with reading !
as meaning "not", but especially if it may be read by people less accustomed to programming, it may make more sense to use the words instead of symbols.
Formatting/Indentation
At least to me, this indentation looks a bit odd:
if (s.insert(n).second == false && m.find(n) == m.end()) {
dups++;
m.insert(pair<int, int>(n,0));
// better to remove from vector than increase space with the map?
// numbers.erase(remove(numbers.begin(), numbers.end(), n), numbers.end());
} else {
s.insert(n);
}
If you use indentation like that consistently, I guess it's not necessarily terrible, but I think more people are accustomed to something more like this:
if (s.insert(n).second == false && m.find(n) == m.end()) {
dups++;
m.insert(pair<int, int>(n,0));
// better to remove from vector than increase space with the map?
// numbers.erase(remove(numbers.begin(), numbers.end(), n), numbers.end());
} else {
s.insert(n);
}
...where each closing brace is vertically aligned with the beginning of the block it closes. As a side-note, there are almost endless debates about the efficacy of various bracing styles. I'm not going to advocate for or against any of the well known styles, but I think there's a fair amount to be gained from using a style that's well known, and then using it consistently. I don't see much to gain from style that's different from what almost anybody else uses.
m.insert(pair<int, int>(n, 0))
can be replaced with simplym.emplace(n, 0)
saving you from writing out the pair constructor. \$\endgroup\$