5
\$\begingroup\$

I'm learning C++ and thought I would try my hand at writing a basic program in modern C++. I'm always looking for ways to reduce my code to STL algorithms and simplifying areas that would require refactoring. I'm also trying to make my code as readable as possible.

Limitation

  • I can't modify the precision in std::to_string as far as I can see, which affects the rest of the program.

What I want

  • Looking for ways to simplify stuff to STL algorithms

  • Ensure everything is modern C++

  • Readability. What if I want to modify the code 3 weeks from now?

  • Anything else?

Code:

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

auto fah_to_cel(double in)
{
    return (in - 32.) / 1.8;
}

int main()
{
    auto min = 0u;
    auto max = 100u;
    auto step = 5u;

    std::vector<double> temps;

    /* size_type is not necessarily the same
     * type as 0u
     */
    auto max_width = std::string::size_type(0);

    for (auto i = min; i < max; i += step)
    {
        auto width = std::to_string(i).size();
        max_width = std::max(width, max_width);
        temps.emplace_back(i);
    }

    auto header_width = sizeof("Fahrenheit") - 1;
    max_width = std::max(header_width, max_width);

    std::cout << "Fahrenheit"
              << std::string(5, ' ')
              << "Celsius"
              << "\n";

    std::cout << std::string(max_width, '-')
              << std::string(5, ' ')
              << std::string(max_width, '-')
              << "\n";

    for (auto& temp : temps)
    {
                  /* Fahrenheit column */
        std::cout << std::left  << std::setw(max_width)
                  << std::fixed << temp

                  /* Padding */
                  << std::string(5, ' ' )

                  /* Celsius column*/
                  << std::left  << std::setw(max_width)
                  << std::fixed << fah_to_cel(temp)
                  << "\n";
    }
}

Example output:

Fahrenheit     Celsius
----------     ----------
0.000000       -17.777778
5.000000       -15.000000
10.000000      -12.222222
15.000000      -9.444444 
20.000000      -6.666667 
25.000000      -3.888889 
30.000000      -1.111111 
35.000000      1.666667  
40.000000      4.444444  
45.000000      7.222222  
50.000000      10.000000 
55.000000      12.777778 
60.000000      15.555556 
65.000000      18.333333 
70.000000      21.111111 
75.000000      23.888889 
80.000000      26.666667 
85.000000      29.444444 
90.000000      32.222222 
95.000000      35.000000 
\$\endgroup\$

3 Answers 3

5
\$\begingroup\$

I see some things that may help you improve your program and have provided answers to your questions.

Be clear about your program's purpose

What the program actually does is to print out a table of temperatures. That much is quite clear, but what is less clear is whether you really wanted the std::vector to be created or not. If that was just a means to the end goal of creating a table, then I'd suggest you could omit it entirely and simply use a loop to print the values rather than storing them.

Use named variables

The word "Fahrenheit" appears twice within the code. It would be better to use named variables rather than risk, say, having a typo in one of the versions. I'd do it like this:

const std::array<std::string, 2>column_labels{"Fahrenheit", "Celsius"};

Remove unecessary data structures

As noted above, the std::vector is not really needed. If the code simply directly prints the values, the code becomes shorter and easier to read and understand.

Eliminate redundant I/O manipulators

Once the width and precision have been set, there's little reason to assert them again. I'd write the main loop like this:

for (double temp=min; temp < max; temp += step) {
    std::cout << std::setw(col_width) << std::fixed 
              << std::setprecision(4) << temp
              << fah_to_cel(temp) << '\n';
}

Note that the limitation of std::to_string is eliminated by this means because we can use std::setprecision().

Avoid construction/destruction overhead

Each time the code calls std::string(5, ' ') it represents a constructor call and somewhere later, a destructor call. This can be avoided either by making it a named const variable which can be repeatedly used, or better still, eliminated entirely by simply using the std::setw() manipulator to set the columns to the size you want, automatically creating the required space.

Let the computer do the work

It seems to make more sense to me to let the computer figure out which label is longest. Here's one simple way to do that:

const auto max_width = std::accumulate(
        column_labels.begin(), column_labels.end(), 0, 
        [](std::string::size_type a, const std::string &str){ 
            return std::max(a, str.size()); 
        });

This uses a lambda to return the longest string length, but this could be done using a function if you wish.

Use const where practical

Things that don't change should be declared const which includes your min, max and many other variables within the code.

Consider whether to use a function or inline

The fah_to_cel routine is quite simple and only used once, so I'd be inclined to eliminate it in favor of simply putting the code inline instead.

An alternative implementation

Using these suggestions, here's an alternative approach:

#include <algorithm>
#include <array>
#include <iomanip>
#include <iostream>

int main()
{
    const auto min = 0u;
    const auto max = 100u;
    const auto step = 5u;      
    const std::array<std::string, 2> column_labels{"Fahrenheit", "Celsius"};
    const auto max_width = std::accumulate(
            column_labels.begin(), column_labels.end(), 0, 
            [](std::string::size_type a, const std::string &str){ 
                return std::max(a, str.size()); 
            });
    const auto col_width = max_width+5;
    std::cout.setf(std::ios_base::left, std::ios_base::adjustfield);
    // print column labels
    for (const auto &label : column_labels) {
        std::cout << std::setw(col_width) << label;
    }
    std::cout << '\n';
    // print column label underlines
    const std::string underline(max_width, '-');
    for (auto i=column_labels.size(); i; --i) {
        std::cout << std::setw(col_width) << underline;
    }
    std::cout << '\n';
    // print temperature table
    for (double temp=min; temp < max; temp += step) {
        std::cout << std::setw(col_width) 
                  << std::fixed << std::setprecision(4) 
                  << temp << (temp-32)/1.8 << '\n';
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ To find the longest string, wouldn't it make more sense to use std::max_element (with a comparator on length) rather than std::accumulate? \$\endgroup\$ Jun 7, 2016 at 19:53
  • \$\begingroup\$ @JerryCoffin: That was my first thought, but std::max_element effectively returns the element rather than the length, so although it's an odd usage, it seemed to me that std::accumulate was slightly more direct. \$\endgroup\$
    – Edward
    Jun 7, 2016 at 20:14
2
\$\begingroup\$

I think you're using "abstraction" in all the wrong places — your code is implicit when it should be explicit, and vice versa.

To start with the simplest case:

auto fah_to_cel(double in)
{
    return (in - 32.) / 1.8;
}

You wrote auto instead of double. Why? To save two characters? The purpose of auto is to allow you to be generic when you need it, not to be sprinkled in whenever you feel like it. This function takes double and returns double; nothing is to be gained here by playing coy with the return type. Just say double; the reader will thank you for not making him trace through the whole function just to figure out its return type.

And you wrote 32. instead of 32 — why? To waste one character? The compiler certainly doesn't care which one you use (in this context). Prefer human-friendly numbers such as "32" to jargon such as "32." (and later, you use "32u") that might again send your reader scrambling for the Standard.

Write the simplest possible thing that will work.


On to your main routine. ...Which is a problem, because you should have written a couple more helper functions instead of cramming everything into main. Your code's flow here is: make a sequence of Fahrenheit temps, then print them out in a formatted table with their Celsius values. This is fine; but your table-formatting logic gets in the way of understanding the basic flow of your program. Let's split that out into a function whose single responsibility is to format the sequences it's given into a table. Perhaps something like this:

void format_as_table(const std::string& header1, const std::vector<double>& data1,
                     const std::string& header2, double (*transform)(double))
{
    auto max_width = std::max(header1.size(), header2.size());  // X
    for (auto& elt : data1) {
        max_width = std::max(max_width, std::to_string(elt).size());
        max_width = std::max(max_width, std::to_string(transform(elt)).size());  // X
    }

    const std::string hyphens(max_width, '-');
    std::cout << header1 << "     " << header2 << "\n";
    std::cout << hyphens << "     " << hyphens << "\n";

    for (const auto& elt : data1) {
        std::cout << std::left << std::setw(max_width) << std::fixed << elt;
        std::cout << "     ";
        std::cout << std::left << std::setw(max_width) << std::fixed << transform(elt) << "\n";
    }
}

I would argue that the lines marked // X are an improvement in the logic; they allow for the possibility that the longest string in the program might be in the second column, in which case (I claim) you'd want to extend the hyphens all the way out to the end of that column. Anyway, if the maintainer disagreed, at least there'd be a single obvious place to change the table-printing logic now.

Then your main would be just something like

int main()
{
    std::vector<double> fahr_temps = generate_range(0, 100, 5);
    format_as_table("Fahrenheit", fahr_temps, "Celsius", fahr_to_cel);
}

Notice that I've replaced your jargon 0u with a plain old human 0, and so on. The signedness of these integral types doesn't matter because you're going to convert them to double before using them anyway; and besides, unsigned is just plain wrong, semantically, for temperatures, except arguably for Kelvins (and even there I'd say there's value in consistently using int for integral values).

Writing generate_range is left as an exercise for the reader.


Having gotten this far, the next step would probably be to generalize format_as_table in one of any number of ways:

  • Don't hardcode std::cout, but allow printing to any ostream& at all.
  • (Probably a bad idea: Don't hardcode ostream, but allow outputting to any OutputIterator at all.)
  • Don't hardcode double (*)(double), but allow transform to be of any type at all.
  • Don't hardcode std::vector<double>, but allow data1 to be of any iterable type at all (or take a pair of iterators, STL-style).

I would start by trying to fill in the body of this function:

template<typename FwdIt, typename Func>
void format_as_table(const std::string& header1, FwdIt begin, FwdIt end,
                     const std::string& header2, const Func& f, ostream& out)
{
    ...
}

and then write the main to go with it.


As for using STL algorithms: yes, I bet there are several places in this code where STL algorithms would be useful. That for-loop that just reduces two sequences with respect to std::max seems like a good candidate for std::accumulate, for example; and I didn't write generate_range for you, but I bet it's a two-liner with the appropriate STL algorithm. (Look up back_inserter and generate_n to get started.)

However, the resulting code is extremely unlikely to be any clearer than the naive for-loop-based version, and probably harder to maintain.. So take STL algorithms with a grain of salt.

Everyone knows that debugging is twice as hard as writing a program in the first place. So if you're as clever as you can be when you write it, how will you ever debug it?

\$\endgroup\$
3
  • \$\begingroup\$ It's quite some overkill to write a general templated format_as_table() function if you're never using it for any other formatted output. Such a generic function makes sense only if you really want to use it generically. \$\endgroup\$
    – Walter
    Jun 7, 2016 at 8:01
  • \$\begingroup\$ @Walter: Agreed. I was guessing from the OP's question that he was using this trivial program as a way to explore STL-ish concepts, decomposition into subroutines, etc etc, even though such a simple task doesn't really require them. If the goal is really to make the best possible temperature-printing program, I'd agree that Edward's answer is better than mine (and I've upvoted accordingly). \$\endgroup\$ Jun 7, 2016 at 22:23
  • \$\begingroup\$ I should also mention that personally I'd use stdio instead of iostreams for formatted output in the first place, which is why I didn't try to do any real refactoring of OP's iostreams manipulators. stdio is generally a better tool than iostreams in that it's declarative (just write the format string and you know your code works) instead of stateful/imperative (set every stream attribute that the last guy might have set differently, then print the thing you want to print, then unset everything you did that might screw up the next guy). However, to each his own. \$\endgroup\$ Jun 7, 2016 at 22:27
-1
\$\begingroup\$

nice code but what get my attention most

auto width = std::to_string(i).size();

width here is equal 3 based on the max if it was converted to string size. also, max_width will be always equal header_width. i don't think you need it here.

use const where it is possible you have few candidate for it like min, max and step. also in for-range loop it is better to consider it for optimizing purpose like so, for (const auto& temp : temps)

for "Fahrenheit" since you going to use it often in your code, it will be nice if create std::string object for it.

std::string name = "Fahrenheit";
const auto max_width = name.length();

there are many ways fill std::vector by using STL one of which is:

std::vector<double> temps;
const auto size = max / step;
temps.reserve(size);
auto i = min;
auto comp = [&i, &step]()
{
    static bool firstRun = true;
    if(firstRun)
    {
         firstRun = false;
        return i;
    }
    else
    {
       return i += step;
    }
};
std::generate_n(std::back_inserter(temps), size, comp);

but personally, simple loop like yours will do the job and much better in term of readability.

\$\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.