patterncppMinorCanonical
Temperature conversion table in C++14
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
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:
Example output:
`Fahrenheit Celsius
---------- ----------
0.000000 -17.777778
5.
Limitation
- I can't modify the precision in
std::to_stringas 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
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:
Remove unecessary data structures
As noted above, the
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:
Note that the limitation of
Avoid construction/destruction overhead
Each time the code calls
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:
This uses a lambda to return the longest string length, but this could be done using a function if you wish.
Use
Things that don't change should be declared
Consider whether to use a function or inline
The
An alternative implementation
Using these suggestions, here's an alternative approach:
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 practicalThings 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.