2
\$\begingroup\$

Is there a way make the code better so that it does not have repeated code?

int totalDistance;

if (totalDistance < pow(10, 3)) {
    cout << "\nTotal (approx) travel distance = " << totalDistance << " million km\n" << endl;
}
else if (totalDistance < pow(10, 6)) {
    totalDistance = totalDistance / pow(10, 3);
    cout << "\nTotal (approx) travel distance = " << totalDistance << " billion km\n" << endl;
}
else if (totalDistance < pow(10, 9)) {
    totalDistance = totalDistance / pow(10, 6);
    cout << "\nTotal (approx) travel distance = " << totalDistance << " trillion km\n" << endl;
}

totalDistance is measured in km and minimum distance is million. My main concern is when totalDistance has reached 1,000 or 1,000,000 then instead "million" change to "billion" or "trillion".

\$\endgroup\$
0

2 Answers 2

4
\$\begingroup\$

Step 1 is always to try it without the duplicated code and see what it might look like; then you can try to fill in the blanks to make the program compile again. So:

cout << "\nTotal (approx) travel distance = "
     << english_approximation(totalDistance) << " km\n" << endl;

And then you just have to write

std::string english_approximation(long long totalDistance)
{
    if (totalDistance < 1e3) {
        return std::to_string(totalDistance) + " million";
    } else if (totalDistance < 1e6) {
        return std::to_string(totalDistance / 1e3) + " billion";
    } else if (totalDistance < 1e9) {
        return std::to_string(totalDistance / 1e6) + " trillion";
    } else {
        assert(false);  // presumably?
    }
}

Are you at all concerned that 1e3, a.k.a. 1000, is not "a million", and so on?


It is unclear to me whether totalDistance is supposed to be measured in kilometers, or meters, or what. (Megameters would make the math come out right, I suppose, but that just seems crazy.) I strongly recommend that any time you're dealing with measurements or other "unit-ful" data, you encode the expected units into the variable name, unless the expected units are already encoded in the variable's type. For example:

std::chrono::milliseconds timeout;  // excellent
int timeout_ms;  // excellent
int timeout;  // horrible

boost::units::kilometers totalDistance;  // excellent (hypothetically; this isn't actually how Boost.Units works)
int totalDistanceKm;  // excellent
int totalDistance;  // horrible
\$\endgroup\$
1
\$\begingroup\$

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

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. I don't know that you've actually done that, but it's an alarmingly common thing for new C++ programmers to do.

Consider more descriptive variable names

From trying the code, I infer that totalDistance is apparently in millions of kilometers. It might be better to either reflect that in the variable name or to at least provide a comment near that variable to point out this essential fact.

Consider a data-centric approach

Rather than repeating things in code, a data structure might be more apppropriate:

void distprint(int totalDistance) {
    constexpr struct {
        int lolimit;
        const char *name;
    } scales[]{
        { 1'000, "million" },
        { 1'000'000, "billion" },
        { 1'000'000'000, "trillion" },
    };
    for (const auto &scale : scales) {
        if (totalDistance < scale.lolimit) {
            std::cout << "\nTotal (approx) travel distance = " 
                << totalDistance/(scale.lolimit/1'000) 
                << ' ' << scale.name << " km \n";
            return;
        }
    }
}

Note that this actually uses the C++14 separator ' for numbers to make them easier to read. If you're stuck with only C++11, you can simply omit them.

Consider validating the input value

What should be printed if the number is negative? What if it's greater than 1000 trillion? At the moment, the first case would print as millions no matter the scale, and the latter wouldn't print anything at all. If that's not what you intend, it is wort looking more closely at how you've implemented this.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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