3
\$\begingroup\$

I have only begun to learn C++ and am reading through the book, "Programming Principles and Practices Using C++". This bit of code is my attempt to complete exercise 3 at the end of chapter 4.

The actual task is:

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.

I am looking for any constructive criticism of my code. Keep in mind some things have not been covered. Even saying that, I still wouldn't mind hearing about what could be. Some things have been covered and I just don't implement them well.

    #include "std_lib_facilities.h"

int main()
{
    double ddistance = 0;
    double dtotaldist = 0;
    double dgreatest = 0;
    double dshortest = 0;
    char cgoagain;
    bool bgoagain = true;
    vector<double>vdistance;

    while (bgoagain) {
        cout << "Enter a valid 'double' ";
        cin >> ddistance;


    if (cin) {
        vdistance.push_back(ddistance);
        dtotaldist += ddistance;
        dshortest = ddistance;

        for (int i = 0; i < vdistance.size(); ++i)
            if ((vdistance[i]) > dgreatest) {
                dgreatest = vdistance[i];
            }
        for (int i = 0; i < vdistance.size(); ++i)
            if (vdistance[i] < dshortest) {
                dshortest = vdistance[i];
            }

        std::cout << "vdistance size is\t" << vdistance.size() << "\n";
        std::cout << "dtotaldist size is\t" << dtotaldist << "\n";
        std::cout << "The mean distance is\t" << dtotaldist / vdistance.size() << "\n";
        std::cout << "The long distance is\t" << dgreatest << "\n";
        std::cout << "The short distance is\t" << dshortest << "\n";
    }

    else {
        std::cout << "That is not a valid value. Would you like to try again? 'y' / 'n' \n";
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
        cin >> cgoagain;
        if (cgoagain == 'y' || cgoagain == 'Y') {
            bgoagain = true;
        }
        else{
            bgoagain = false;
        }
    }


    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Is "std_lib_facilities.h" something provided by the book? It's probably better to include a small set of standard headers, so that anyone can compile and test your code. \$\endgroup\$ – Toby Speight Oct 1 '18 at 8:30
5
\$\begingroup\$

I guess std_lib_facilities is a header from the book? It's best to get used to including the standard library headers directly as soon as possible.

If std_lib_facilities contains using namespace std; note that this is bad practice.

In this case, we only need:

#include <vector>
#include <iostream>

"Hungarian" notation (adding type information to the names of variables), is not generally recommended. As such, the variable names should not be ddistance, cgoagain etc., just distance and goagain.

Using camel case or underscores may make them easier to read, and it's generally better to use whole words instead of abbreviations, e.g. totalDistance.


While C once had a requirement for all variables to be declared at the start of a block, C++ does not. Variables should be declared as close to their point of use as practical. As such, ddistance and cgoagain can be moved to just above where they are populated from std::cin.


Since you're storing the greatest and shortest distances, there's no need to iterate the whole vector of distances when a new value is entered. We can simply compare against the existing value. However, this won't work when initializing the greatest and shortest values to zero!

We can use another part of the standard library to initialize the greatest value to something that will always be smaller than a valid double input, and the shortest value to something that will always be larger. That way the first input will override both values successfully:

#include <limits>
...
double greatest = std::numeric_limits<double>::lowest();
double shortest = std::numeric_limits<double>::max();

...
        if (distance > greatest)
            greatest = distance;

        if (distance < shortest)
            shortest = distance;

Note that with this change, there's actually no need to keep a vector containing all the distances. We can just keep a count of the number of values entered and use that to calculate the mean.


Altered code:

#include <vector>
#include <iostream>
#include <limits>

int main()
{
    double totalDistance = 0;
    double greatest = std::numeric_limits<double>::lowest();
    double shortest = std::numeric_limits<double>::max();
    unsigned int distancesEntered = 0;

    while (true) {

        std::cout << "Enter a valid 'double' ";

        double distance = 0;
        std::cin >> distance;

        if (std::cin) {

            ++distancesEntered;

            totalDistance += distance;

            if (distance > greatest)
                greatest = distance;

            if (distance < shortest)
                shortest = distance;

            std::cout << "distancesEntered is\t" << distancesEntered << "\n";
            std::cout << "totalDistance is\t" << totalDistance << "\n";
            std::cout << "The mean distance is\t" << totalDistance / distancesEntered << "\n";
            std::cout << "The greatest distance is\t" << greatest << "\n";
            std::cout << "The shortest distance is\t" << shortest << "\n";
        }

        else {
            std::cout << "That is not a valid value. Would you like to try again? 'y' / 'n' \n";
            std::cin.clear();
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');

            char goAgain = 0;
            std::cin >> goAgain;

            if (!(goAgain == 'y' || goAgain == 'Y')) {
                break;
            }
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ B. Stroustrup had to take some tradeoffs (with respect to best practices for the production of professional code) in that book where he teaches C++ as an introductory language, because learning to program is difficult even with a simple language, and C++ is a very complex one. Following blindly "best practice" would be to never write min and max functions because you have them in the library. \$\endgroup\$ – Michel Billaud Oct 1 '18 at 9:20
  • \$\begingroup\$ Perhaps. I don't think anyone should blindly follow anything. "Best practice" is just guidelines to help accomplish things easily and avoid potential pitfalls. The links above have more information on the "whys" of what I've recommended. \$\endgroup\$ – user673679 Oct 1 '18 at 14:19
  • 1
    \$\begingroup\$ My comment was more precisely directed to the "using namespace std considered bad practice". Anyway, the original code is not consistent in its notation (with/without std). Consistency is a good practice. \$\endgroup\$ – Michel Billaud Oct 1 '18 at 14:27
3
\$\begingroup\$

I'm not familiar with the book, however, as you are at chapter 4 I will most likely tell you about stuff that will still follow.

Hungarian notation

Looking at the names of your variables, you seem to have your own version of the Hungarian notation. I've learned from experience that using it doesn't add much benefits. It makes it hard to change types, especially when you start using auto. When types start to become more complex, it becomes really confusing, where introducing new types is a better solution than encoding the type in the name.

Functions

Currently, all of your code is in the single function main. I would separate the concerns of the logic into multiple functions:

  • loadData: Function that retrieves the data and builds up the vector. No cout, no calculations.
  • use STL algorithms: auto totalDistance = std::accumulate(distance.cbegin(), distance.cend(), 0.); or 'auto [shortestIterator, greatestIterator] = std::minmax_element(distance.cbegin(), distance.cend());'
  • printOutput: Print the output of the calculations
\$\endgroup\$
2
\$\begingroup\$

The exercise "suggests" you first store the data into a vector (sentence #1), then compute and display some results.

You should define separate functions for the actions : input data, compute sum, compute minimum, compute average etc. See 4.5.1 from your book: "Why bother with functions".

With a correct decomposition, the main() function would look like

int main() 
{  
    std::vector<double> distances;
    get_data(distances);

    std::cout << "size is\t"  << distances.size()   << std::endl
              << "total is\t" << sum_of(distances)  << std::endl
              << "min is\t"   << ....
              ;
    return 0;
}

ALSO, you should indent your code properly. It will show you why it doesn't work as required (instead you print the "final" results inside the input loop, each time a value is given).

EDIT 2 : If you follow your initial idea (processing data as soon as you get it), you don't have to store it to a vector, because you can compute sum, min and max "on the fly"

number_of_values += 1;
sum_do_far += new_value;
if (new_value > maximum_so_far) {
     maximum_so_far = new_value;
}
if (new_value < minimum_so_far) {
     minimum_so_far = new_value;
}

(and then the exercise falls flat)

\$\endgroup\$

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.