HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinorCanonical

Temperature conversion table in C++14

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
conversiontemperaturetable

Problem

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 
#include 
#include 
#include 

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

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

    std::vector 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.

Solution

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::arraycolumn_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 
#include 
#include 
#include 

int main()
{
    const auto min = 0u;
    const auto max = 100u;
    const auto step = 5u;      
    const std::array 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';
    }
}

Code Snippets

const std::array<std::string, 2>column_labels{"Fahrenheit", "Celsius"};
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';
}
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()); 
        });
#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';
    }
}

Context

StackExchange Code Review Q#131278, answer score: 5

Revisions (0)

No revisions yet.