3
\$\begingroup\$

Read a sequence of double values into a vector. Think of each value as the distance between two cities along a given route. Compute and print the total distance (the sum of all distances). Find and print the smallest and greatest distance between two neighboring cities. Find and print the mean distance between two neighboring cities.

from Programming Principles and Practice using C++ by Bjarne Stroustrup

Since always entering values seems stupid, I decided to read the values from a file named "distances.txt".

print_distances_mean_max_min is what concerns me, how could I split the computing of min,max,mean and printing it while not spreading the code too thinly? Or is it fine as it is?

main.cpp

#include <iostream>
#include <vector>
#include <fstream>
#include <stdexcept>
#include <algorithm>

using std::vector;
using std::string;
using std::fstream;
using std::ios_base;
using std::runtime_error;
using std::cout;
using std::max_element;
using std::min_element;

const string distances_file_name = "distances.txt";

vector<double> load_file_of_doubles_to_vector ( string file_name )
{
    vector<double> distances;
    fstream my_file ( file_name, ios_base::in );
    double temporary_number;
    while ( my_file >> temporary_number )
    {
        distances.push_back ( temporary_number );
    }
    if ( !my_file.is_open() )
    {
        throw runtime_error ( string ( "Failed to load " ) + file_name );
    }
    return distances;
}

void print_distances_mean_max_min ( const vector<double>& distances_vector )
{
    if ( distances_vector.empty() )
    {
        throw runtime_error ( string ( "There are no entries in " ) + distances_file_name );
    }
    double total_distance = 0;
    for ( auto& n : distances_vector )
    {
        total_distance += n;
    }
    cout << "The sum of all distances is: " << total_distance << "\n";
    double max_distance = 0;
    max_distance = *max_element ( distances_vector.begin(), distances_vector.end() );
    cout << "The maximum distance is: " << max_distance << "\n";
    double min_distance = 0;
    min_distance = *min_element ( distances_vector.begin(), distances_vector.end() );
    cout << "The minimum distance is: " << min_distance << "\n";
    double mean_distance = 0;
    mean_distance = total_distance / distances_vector.size();
    cout << "The average(mean) distance is: " << mean_distance << "\n";
}

int main()
{
    print_distances_mean_max_min ( load_file_of_doubles_to_vector ( distances_file_name ) );
    return 0;
}

distances.txt

8.7
6.3
9.0
3.142
2.7
\$\endgroup\$
1
  • \$\begingroup\$ In Java, I would create my own type to throw, and I'd use a checked exception, not a run time error. But the protocol in C++ might be different, it's something to look into. \$\endgroup\$
    – markspace
    Mar 6 '20 at 22:37
3
\$\begingroup\$

Here are some suggestions for you to improve your code.


#include <iostream>
#include <vector>
#include <fstream>
#include <stdexcept>
#include <algorithm>

Sort the #include directives in alphabetical order. This helps navigation.


using std::vector;
using std::string;
using std::fstream;
using std::ios_base;
using std::runtime_error;
using std::cout;
using std::max_element;
using std::min_element;

This is much better than using namespace std;, but I still suggest getting into the habit of explicitly qualifying the names with std:: as soon as possible (especially for those names which you only use once, e.g., max_element). For small programs like this it doesn't matter, but for larger programs, qualification adds clarify — when I see string, does it mean std::string? Jewelry::string, a string of pearls? Or Minecraft::string, used to craft bows and fishing rods? This problem is especially evident for common identifiers, like count, arg, or data — these are all top level names in std!

Moreover, using unqualified names sometimes cause ADL (argument dependent lookup), an additional phase for name lookup, to kick in. As a result, an attempt to call a function in the standard library may end up inadvertently calling a function in an irrelevant namespace. Such problems come up often in large programs, so again, it's better to get into the habit of using std::.

It is acceptable to use using declarations or derivatives in reduced scopes, to reduce clutter. But using them globally for the whole program is likely to cause problems. Moreover, std:: is only 5 characters — reducing clutter is more relevant for things like

boost::math::double_constants::pi

or

boost::multiprecision::number<
    boost::multiprecision::mpfr_float_backend<300>,
    boost::multiprecision::et_off
>;

const string distances_file_name = "distances.txt";

Use constexpr std::string_view (defined in header <string_view>) for string constants:

constexpr std::string_view file_name{"distances.txt"};

or

using namespace std::literals;
constexpr auto file_name = "distances.txt"sv;

vector<double> load_file_of_doubles_to_vector ( string file_name )
{
    vector<double> distances;
    fstream my_file ( file_name, ios_base::in );
    double temporary_number;
    while ( my_file >> temporary_number )
    {
        distances.push_back ( temporary_number );
    }
    if ( !my_file.is_open() )
    {
        throw runtime_error ( string ( "Failed to load " ) + file_name );
    }
    return distances;
}

The parameter type should be std::string_view to avoid redundant copies.

The file stream should be of type std::ifstream instead of std::fstream because it is used for input only. Then, you can omit the in option, which is the default for std::ifstream.

string ( "Failed to load " ) + file_name is too verbose. Use "Failed to load "s.

Consider using std::istream_iterator, std::back_inserter (both defined in <iterator>), and std::copy to read the values:

// requires NTBS as argument
auto load_file(std::string_view file_name)
{
    // std::string_view doesn't have c_str()
    std::ifstream in{file_name.data()};
    if (!in.is_open()) {
        // assuming 'using namespace std::literals;'
        throw std::runtime_error{"the file doesn't exist"};
    }

    std::vector<double> data;
    std::copy(std::istream_iterator<double>{in},
              std::istream_iterator<double>{},
              std::back_inserter(data));
    return data;
}

void print_distances_mean_max_min ( const vector<double>& distances_vector )
{
    if ( distances_vector.empty() )
    {
        throw runtime_error ( string ( "There are no entries in " ) + distances_file_name );
    }
    double total_distance = 0;
    for ( auto& n : distances_vector )
    {
        total_distance += n;
    }
    cout << "The sum of all distances is: " << total_distance << "\n";
    double max_distance = 0;
    max_distance = *max_element ( distances_vector.begin(), distances_vector.end() );
    cout << "The maximum distance is: " << max_distance << "\n";
    double min_distance = 0;
    min_distance = *min_element ( distances_vector.begin(), distances_vector.end() );
    cout << "The minimum distance is: " << min_distance << "\n";
    double mean_distance = 0;
    mean_distance = total_distance / distances_vector.size();
    cout << "The average(mean) distance is: " << mean_distance << "\n";
}

IMO, this function is unnecessary and can be built into main directly, because it doesn't have a coherent logic — it just shows what the program does.

Catching exceptions in the main function and printing the messages helps the end user know what kind of error happened.

Don't reinvent the wheel — use std::accumulate or std::reduce (defined in header <numeric>) to calculate sums.

You can calculate the minimum and maximum values simultaneously in \$\lfloor 3(N-1)/2 \rfloor\$ comparisons using std::minmax_element, which returns a std::pair of iterators pointing to the minimum and maximum elements respectively. Then, we can use a structured binding to conveniently obtain the two iterators and dereference them to get the values:

int main()
try {
    auto data = load_file(file_name);
    if (data.empty()) {
        throw std::runtime_error{"no entries"};
    }

    auto sum = std::accumulate(data.begin(), data.end(), 0.0);
    auto mean = sum / data.size();
    auto [min_it, max_it] = std::minmax_element(data.begin(), data.end());

    std::cout << "The sum of all distances is: " << sum << '\n'
              << "The maximum distance is: " << *max_it << '\n'
              << "The minimum distance is: " << *min_it << '\n'
              << "The average (mean) distance is: " << mean << '\n';
} catch (std::runtime_error& error) {
    std::cerr << "Runtime error: " << error.what() << '\n';
    return 1;
} catch (...) {
    std::cerr << "Unknown error\n";
    return 2;
}

I've made an online demo by putting everything together so that you can play with the code.

\$\endgroup\$
5
  • \$\begingroup\$ Hi, thanks for your answer. What are "ADL problems"? Doesn't add "using std::string" a little readability, especially if you got for example std::vector<std::string> ? Which type "auto [min_it, max_it]" will get, and why use "auto"? Why does putting a function into the main add more ability to catch errors, can't I do the "try"-block inside a function? Bonuspoints for showing me Wandbox, this site is neat. \$\endgroup\$
    – SAJW
    Mar 7 '20 at 12:38
  • \$\begingroup\$ @SAJW You're welcome. I've updated the answer address your questions. The part about catching exceptions in the main function was worded poorly; I've tried clarify it - where you catch the exceptions is not important in this case, as long as you catch them. \$\endgroup\$
    – L. F.
    Mar 7 '20 at 13:30
  • \$\begingroup\$ I think you got a typo here "when I see vector, does it mean std::string? " i think you mean: "when I see string..." or? Also I'm not quite sure if alphabeticly ordering of the #includes is what I want, see the comments in this topic: stackoverflow.com/questions/60578003/… \$\endgroup\$
    – SAJW
    Mar 7 '20 at 14:15
  • \$\begingroup\$ You asked a very good question in the link you provided "but if the order of the #includes does matter, is this not a sign that at least one header is written badly and should be changed?" Yes, it does signify that if there is dependency. With these standard headers, there is no such dependency. \$\endgroup\$ Mar 7 '20 at 16:23
  • \$\begingroup\$ @SAJW That's indeed a typo - thanks for spotting! Alphabetically ordering headers is just a general guideline; there are (rare) cases where include order is important, but this is not the case for standard headers (which are just regular and self-contained headers). So you probably shouldn't let your editor do it automatically. It's common for large programs to require many headers in one translation unit, so sorting is still beneficial for readability. \$\endgroup\$
    – L. F.
    Mar 8 '20 at 2:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.