Finding max, min, mode

Question

Could you review my code for finding max, min, and mode?

#include <iostream>
#include <vector>
#include <algorithm>
int main()
{
 std::vector <int> v = { 2, 5, 3, 4, 3, 2, 3, 3, 3 , 6, 6, 6, 6, 6, 6, 6, 6, 1};
 sort(v.begin(), v.end());
 int number = v[0];
 int max = v[0];
 int min = v[0];
 int mode;
 for (int i = 1, countMode = 1, count = 1; i < v.size(); ++i) {
 if (v[i] == number)
 ++countMode;
 if (countMode > count) {
 count = countMode;
 mode = number;
 }
 if (v[i] > max)
 max = v[i];
 if (v[i] < min)
 min = v[i];
 number = v[i];
 }
 std::cout << max << ' ' << min << ' ' << mode;
}

Answer 1

Here are some things that may help you improve your code.

Use direct initialization

In the first line, we have this:

std::vector <int> v = { 2, // ...

However, this may be shortened slightly by simply omitting the =:

std::vector <int> v{ 2, // ...

This is list initialization which was standardized in C++11.

Use "range-for" to simplify the loop

Instead of using this somewhat complicated for loop:

for (int i = 1, countMode = 1, count = 1; i < v.size(); ++i) {

You could instead use a range-for:

for (const auto n : v) {

Avoid doing needless calculations

By definition, in a sorted list the first number will be min and the last max, so there's not much point in making those calculations.

Use better naming

Variables min and max are good because they give a good clue as to what they hold. However, number might be better called prev since it's the previous number in the sorted list.

Here's one way to do all of that:

int max = v.back();
int min = v.front();
int prev = max;
int mode;
int maxcount = 0;
int currcount = 0;
for (const auto n : v) {
 if (n == prev) {
 ++currcount;
 if (currcount > maxcount) {
 maxcount = currcount;
 mode = n;
 }
 } else {
 currcount = 1;
 }
 prev = n;
}

Answer 2

Your code is wrong, since you never set countMode to 0 when number changes. Furthermore, finding the minimum and maximum of an ordered vector is \$ \mathcal O(1)\$:

const auto min = v.front();
const auto max = v.back();

For the mode, you need to reset countMode:

for (int i = 1, countMode = 0, count = 0; i < v.size(); ++i) {
 if (v[i] == number) {
 // we're still using the same number
 ++countMode;
 } else {
 if (countMode > count) {
 count = countMode;
 mode = number;
 }
 countMode = 1;
 number = v[i];
 }
}
// some code missing

Note that you need to take care of the last group of numbers. Even better, traverse the vector group-wise:

for(int i = 0; i < v.size(); ++i) {
 const auto number = v[i];
 const auto start = i;
 while(v[i] == number && i < v.size())
 ++i;
 const auto countMode = i - start;
 if(coundMode > count){
 mode = number;
 countMode = count;
 }
}

Other than that, your code fine. However, I would probably create a function that handles this instead, e.g.

template <class T>
stats<T> statistics(std::vector<T> values);

but that's probably an overkill.

• Herman Tam: Why would I need to initialize countMode and count to zero?
• Zeta: You need to reset countMode whenever number changes. Whether you reset it to 0 or 1 depends on your other logic.

Answer 3

#include <iostream>
#include <vector>
#include <algorithm>

In smaller programs like yours, traversing a list of 3 #includes isn't an issue. In larger programs, the list of #included libraries can become large. Organizing your #includes into logical groups and alphabetizing those subgroups helps determine if something is #included. Think about how you search a phone book. You typically use some pivoted search to narrow down your search range. The same applies when looking at a list of library #includes.

---

sort(v.begin(), v.end());

Unless your intent is to invoke argument dependent lookup (ADL), I would suggest you qualify your calls. As the reader, I'm left to try to figure out if this is a call to std::sort or your own rolled sort. ADL can also result in an unintended function being called.

If your intent was to use ADL, be explicit with a using-declaration.

using std::swap; // Explicit: opting into ADL.
swap(lhs, rhs); // unqualified lookup, find the best match.

---

for (int i = 1, countMode = 1, count = 1; i < v.size(); ++i) {
 if (v[i] == number)
 ++countMode;
 if (countMode > count) {
 count = countMode;
 mode = number;
 }
}

It should be noted that the calculation for the mode is incorrect. You never reset the countMode value, so your function returns the last duplicate value seen rather than the mode.

Assuming you fix that, consider other possible sequence structures.

• Many duplicates - where mode([1, 1, 2, 2, 3, 3]) should result in the multimodal set [1, 2, 3].
• Unique values - where mode([1, 2, 3]) should result in the empty set [].
• Empty set - where mode([]) should result in the empty set [].

Consider how your program handles these situations.

In the case of an empty set, v[0] doesn't exist. Attempting to access v[0] produces a segmentation fault. mode is also never initialized and your program will exhibit undefined behavior if you try to do anything with that value.

---

While it is easy for beginner's to just dump everything into main(), I would suggest you break your program up into smaller, focused, and testable logical abstractions. min and max elements are as simple as calls to front() and back() on sorted lists. If you want those values for unsorted lists, the standard library provides std::min_element, std::max_element and std::minmax_element in the <algorithm> library. mode should be abstracted into it's own function as well utilizing other abstractions to accomplish its task.