Mapping & Sorting Randomly Generated Numbers

Question

Below is the assignment I was given and the coding is what I have came up with. I would appreciate criticism on how I did and on ways to have gone differently about it.

You want to number your random list numbers, so use an iterator to put the list numbers into a map (but keep the list). The key will be the number the random number was generated in starting at 1 (ex. 1, 2, 3, etc., since these are not just index numbers). The value will be the random number. Use an iterator to print the two fields lined up on the right with field widths in two columns with the numbers labeled with centered column headings of Key and Value.

Since the list is easier to work with, lets work with the list to perform some calculations. You will sort the numbers in your list. Then use iterators, algorithms, and list functions (not the user's input variable) to find the sum, mean (average), median (middle value - find the size and move up half way), range (difference between highest and lowest), and a random number from a random spot from your list of random number (get a random number (in the proper range) with rand() and move up that many spots in your list). Then print the labeled values (with the mean to three decimal places).

Now, use your iterator to put the now sorted list numbers back into the map (which will change the original map's order). The key will still be numbers starting at 1. The value will now be the random number in their sorted order. Again, use an iterator to print the two fields lined up on the right with field widths in two columns with the numbers labeled with column headings of Key and Value.

#include <bits/stdc++.h>
using namespace std;

int main() {

    cout << "Enter the length of list: ";
    int n;
    cin >> n;

    std::vector<int> v(n);

    cout << "The random generated numbers are: \n";

    for (int i = 0; i < n; i++) {
        int x = rand() % 151;
        cout << std::setw(10);
        cout << i << " ";
        cout << std::setw(10);
        cout << x << endl;
        v[i] = x;
    }
    int sum = 0;
    for (int i = 0; i < n; i++)
        sum += v[i];

    cout << "The sum: " << sum << endl;
    cout << "The mean: " << sum / n*1.0 << endl;

    sort(v.begin(), v.end());
    cout << "The median: " << v[n / 2] << endl;

    return 0;
}

Answer 1

Include the right things

You're starting with:

#include <bits/stdc++.h>
using namespace std;

The second line is bad practice; avoid it. Typing std:: is not a burden. The former is definitely unacceptable - that's a compiler-specific file that you don't want to be including. Just include the actual headers you need:

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

Use algorithms and iterators

Rather than writing your own loop to perform a sum, you could use the standard algorithm for this: std::accumulate:

int sum = std::accumulate(v.begin(), v.end(), 0);

Also they wanted you to use iterators to find the median, which would be:

auto median_it = std::next(
    v.begin(),
    std::distance(v.begin(), v.end()) / 2);

Where's the rest?

There's a lot of text in that question that I don't think you addressed in your code. Looks like we need a std::map or something.

Answer 2

Ensure the minimum required libraries are included

bits/stdc++.h is not standard and not portable. Considering this is an assignment, your goal should be to write portable code so the code works anywhere (home, lab, lecturer's office). Just include the necessary libraries instead of relying on an implementation defined header.

using namespace std; is considered bad practice

using-directives could introduce errors related to ambiguity or symbol-clashing. Prefixing std:: actually helps express intent to your readers and to the compiler. Prefer to prefix unless you intend to use using-declarations (using std::swap; swap(...);) for ADL.

Verify input

Make sure any input you request from the user is validated and invalid inputs are handled appropriately. Consider what would happened if someone inputted negative or non-integer values.

rand() considered harmful

There are better tools for generating random numbers these days than the classic rand(). Use the <random> library.

Separate logic from presentation

When loops start doing more than one thing, readability begins to take a hit. Split your loops into distinct sections that focuses on a singular concern (separation of concerns).

// Generate list
for (int i = 0; i < n; i++) {
    int x = rand() % 151;
    v[i] = x;
}
// Print list
for (int i = 0; i < n; i++) {
    cout << std::setw(10);
    cout << i << " ";
    cout << std::setw(10);
    cout << v[i] << endl;
}

Avoid "magic" constants

Use symbolic constants as they offer semantic meaning to a statement, clearing up any confusion. In your printing of the list, 151 and 10 are both magic constants. Consider giving them appropriate symbolic names.

No raw for loops

Rarely should be the case where you actually need to use a raw for loop. Use algorithms to do the looping for you or use a range-based for loop.

int sum = 0;
for (int i : v) {
    sum += i;
}

Know your <algorithm>s

A majority of your code could be written using standard algorithms. You can use std::nth_element to find the median std::generate to create the elements for your list . You could use std::accumulate to find the sum (and remember to include <numeric>), but you could also easily use a range-based for instead.

Completing the assignment

Missing is the mapping of the 1-based index to the random values in your list. When you print, you are basing the index from 0: this would be incorrect according to the specifications.