patterncppMinor
Finding max, min, mode
Viewed 0 times
minmodemaxfinding
Problem
Could you review my code for finding max, min, and mode?
#include
#include
#include
int main()
{
std::vector 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 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;
}Solution
Here are some things that may help you improve your code.
Use direct initialization
In the first line, we have this:
However, this may be shortened slightly by simply omitting the
This is list initialization which was standardized in C++11.
Use "range-for" to simplify the loop
Instead of using this somewhat complicated
You could instead use a range-for:
Avoid doing needless calculations
By definition, in a sorted list the first number will be
Use better naming
Variables
Here's one way to do all of that:
Use direct initialization
In the first line, we have this:
std::vector v = { 2, // ...However, this may be shortened slightly by simply omitting the
=:std::vector 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;
}Code Snippets
std::vector <int> v = { 2, // ...std::vector <int> v{ 2, // ...for (int i = 1, countMode = 1, count = 1; i < v.size(); ++i) {for (const auto n : v) {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;
}Context
StackExchange Code Review Q#138753, answer score: 8
Revisions (0)
No revisions yet.