Modernizing
There are two things here that most modern C++ programs try to avoid: function pointers (pointers in general, really) and complex macros. Replacing function pointers is easy since the introduction of the <functional> library. The expression double (*toKelvin)(double); can be replaced with std::function<double(double)> toKelvin;. In the template parameter (the stuff between the angle brackets <>), the first double is the return type while the double in the parentheses is the argument list. So, the TemperatureConverter class turns into this:
#include <string>
#include <vector>
#include <functional>
class TemperatureConverter {
public:
std::function<double(double)> toKelvin;
std::function<double(double)> fromKelvin;
static TemperatureConverter* get(string name) {
return converters[name];
}
TemperatureConverter(
std::function<double(double)> toKelvin,
std::function<double(double)> fromKelvin,
vector<string> names
) : toKelvin(toKelvin), fromKelvin(fromKelvin) {
for(auto name : names)
converters[name] = this;
}
private:
static map<string, TemperatureConverter*> converters;
};
As for macros, they are hard to read and easy to mess up, especially multi-line macros like you have. The fact that macros operate on text substitution makes type-safety harder to enforce. In your macro, all you're doing is declaring an instance of each conversion. So, let's just use the constructor of the TemperatureConverter class. We can use lambda expressions in the arguments.
First, instead of the macro
TEMPERATURE_CONVERTER(kelvin, kelvin, kelvin, "Kelvin", "K", "k");
Let's use the constructor of TemperatureConverter:
TemperatureConverter([](double kelvin) { return kelvin; }, [](double kelvin) { return kelvin; }, {"Kelvin", "K", "k"})
The expression [](double kelvin) { return kelvin; } is a lambda expression. The general form is [captured variables](argument variables) { function body }. The lambda expression in the constructor argument captures no variables, takes one double argument and returns that argument unchanged since this is the Kelvin to Kelvin conversion. The final argument {"Kelvin", "K", "k"} is an initializer_list that will be used to construct the vector<string> argument (see #10 on that page). The Celsius converter would look like this:
TemperatureConverter([](double celsius) { return celsius + 273.15; } , [](double kelvin) { return kelvin - 273.15; } , {"Celsius", "°C", "C", "c"})
The TemperatureConverter instances created by your macro are never used. All of the operations are done through the static calls. So, since we only need these instances to exist for the duration of the program so that the pointers in TemperatureConverter::converters remain good, let's just put them in an array and not bother with naming them:
const auto conversions = {
TemperatureConverter([](double kelvin) { return kelvin; }, [](double kelvin) { return kelvin; }, {"Kelvin", "K", "k"}),
TemperatureConverter([](double celsius) { return celsius + 273.15; } , [](double kelvin) { return kelvin - 273.15; } , {"Celsius", "°C", "C", "c"}),
TemperatureConverter([](double delisle) { return 373.15 - delisle * 2 / 3; } , [](double kelvin) {return (373.15 - kelvin) * 3 / 2; } , {"Delisle", "°De", "De", "DE", "de"}),
// etc.
}
The keyword auto tells the compiler to make the type of conversions the same as the result of the expression on the right of the equals sign. In this case, const TemperatureConverter[] but with a lot less typing.
Putting all this together, here's the first iteration of the modernized program:
#include <iostream>
#include <map>
#include <string>
#include <vector>
#include <functional>
using namespace std;
class TemperatureConverter {
public:
std::function<double(double)> toKelvin;
std::function<double(double)> fromKelvin;
static TemperatureConverter* get(string name) {
return converters[name];
}
TemperatureConverter(
std::function<double(double)> toKelvin,
std::function<double(double)> fromKelvin,
vector<string> names
) : toKelvin(toKelvin), fromKelvin(fromKelvin) {
for(auto name : names)
converters[name] = this;
}
private:
static map<string, TemperatureConverter*> converters;
};
map<string, TemperatureConverter*> TemperatureConverter::converters;
const auto conversions = {
TemperatureConverter([](double kelvin) { return kelvin; }, [](double kelvin) { return kelvin; }, {"Kelvin", "K", "k"}),
TemperatureConverter([](double celsius) { return celsius + 273.15; } , [](double kelvin) { return kelvin - 273.15; } , {"Celsius", "°C", "C", "c"}),
TemperatureConverter([](double delisle) { return 373.15 - delisle * 2 / 3; } , [](double kelvin) {return (373.15 - kelvin) * 3 / 2; } , {"Delisle", "°De", "De", "DE", "de"}),
// etc.
};
int main(const int argc, const char* const argv[]) {
auto toKelvin = TemperatureConverter::get(argv[1])->toKelvin;
auto fromKelvin = TemperatureConverter::get(argv[2])->fromKelvin;
for(int i = 3; i < argc; i++)
cout << fromKelvin(toKelvin(atof(argv[i]))) << endl;
return EXIT_SUCCESS;
}
Structuring
The current TemperatureConverter class is doing two things: (1) defining conversions to and from Kelvin, and (2) storing references to all other conversions. Your program would be better structured if these two operations were separated. For example, here's a class that will hold all of the conversions:
class ArbitraryTemperatureConverter
{
public:
void add(std::function<double(double)> toKelvin,
std::function<double(double)> fromKelvin,
const vector<string>& names) {
const auto converter = TemperatureConverter{toKelvin, fromKelvin};
for(const auto& name : names) {
converters.emplace(name, converter);
}
}
double convert(double temperature, const std::string& fromTempName, const std::string& toTempName) const {
const auto& toKelvin = get(fromTempName).toKelvin;
const auto& fromKelvin = get(toTempName).fromKelvin;
return fromKelvin(toKelvin(temperature));
}
private:
map<string, TemperatureConverter> converters;
const TemperatureConverter& get(const string& name) const {
return converters.at(name);
}
};
Reference for std::map::insert() which takes a key-value pair as an argument
Notice: no need for static, no need for pointers, no need for unused variable names. And, since this class handles conversions from temperature scale names to converters, we can delete that part of the TemperatureConverter class.
class TemperatureConverter {
public:
std::function<double(double)> toKelvin;
std::function<double(double)> fromKelvin;
TemperatureConverter(
std::function<double(double)> toKelvin,
std::function<double(double)> fromKelvin
) : toKelvin(toKelvin), fromKelvin(fromKelvin) {}
};
This frees up your main() function to just handle command line arguments. Plus, this makes it easier to add more temperature conversions in other parts of your code by passing an instance around.
int main(const int argc, const char* const argv[]) {
auto all_conversions = ArbitraryTemperatureConverter();
all_conversions.add([](double kelvin) { return kelvin; }, [](double kelvin) { return kelvin; }, {"Kelvin", "K", "k"});
all_conversions.add([](double celsius) { return celsius + 273.15; }, [](double kelvin) { return kelvin - 273.15; }, {"Celsius", "°C", "C", "c"});
all_conversions.add([](double delisle) { return 373.15 - delisle * 2 / 3; }, [](double kelvin) {return (373.15 - kelvin) * 3 / 2; }, {"Delisle", "°De", "De", "DE", "de"});
// etc.
for(int i = 3; i < argc; i++)
cout << all_conversions.convert(stod(argv[i]), argv[1], argv[2]) << endl;
return EXIT_SUCCESS;
}
Other bits
Passing by reference
For variables that are more complex that int, double, char, and the like. You should pass these by const-reference (const string&) instead of by value (string). The latter makes an unnecessary copy of the data. Instead of
static TemperatureConverter* get(string name) {
return converters[name];
}
write
static TemperatureConverter* get(const string& name) {
return converters[name];
}
Instead of
TemperatureConverter(
std::function<double(double)> toKelvin,
std::function<double(double)> fromKelvin,
vector<string> names
) : toKelvin(toKelvin), fromKelvin(fromKelvin) {
for(auto name : names)
converters[name] = this;
}
write
TemperatureConverter(
std::function<double(double)> toKelvin,
std::function<double(double)> fromKelvin,
const vector<string>& names // <---
) : toKelvin(toKelvin), fromKelvin(fromKelvin) {
for(const auto& name : names) // <---
converters[name] = this;
}
Return references instead of pointers (applies only to old code structure)
Since nullptr is a valid value of a pointer variable, users will have to check for nullptr before using the returned pointer. You could document that the returned pointer is never null, but it would be better if that guarantee was enforced by the compiler. In C++, references are never allowed to be null.
static const TemperatureConverter& get(const string& name) {
return *converters[name];
}
I'm returning a const reference here to prevent the toKelvin and fromKelvin functions from being modified.
The calling code will now look like this (the -> have changed to .):
auto toKelvin = TemperatureConverter::get(argv[1]).toKelvin;
auto fromKelvin = TemperatureConverter::get(argv[2]).fromKelvin;
Use std::stod instead of atof
If atof is given a string that cannot be parsed as a floating point value, it will return 0.0. You have no way of knowing if the call succeeded. If you use std::stod from the <string> library, then an exception is thrown for unparseable values. Since 0.0 is a valid temperature, atof will give no hint that there was bad input.
Everything together
The final version of this code:
#include <iostream>
#include <map>
#include <string>
#include <vector>
#include <functional>
using namespace std;
class TemperatureConverter {
public:
std::function<double(double)> toKelvin;
std::function<double(double)> fromKelvin;
TemperatureConverter(
std::function<double(double)> toKelvin,
std::function<double(double)> fromKelvin
) : toKelvin(toKelvin), fromKelvin(fromKelvin) {}
};
class ArbitraryTemperatureConverter
{
public:
void add(std::function<double(double)> toKelvin,
std::function<double(double)> fromKelvin,
const vector<string>& names) {
const auto converter = TemperatureConverter{toKelvin, fromKelvin};
for(const auto& name : names) {
converters.emplace(name, converter);
}
}
double convert(double temperature, const std::string& fromTempName, const std::string& toTempName) const {
const auto& toKelvin = get(fromTempName).toKelvin;
const auto& fromKelvin = get(toTempName).fromKelvin;
return fromKelvin(toKelvin(temperature));
}
private:
map<string, TemperatureConverter> converters;
const TemperatureConverter& get(const string& name) const {
return converters.at(name);
}
};
int main(const int argc, const char* const argv[]) {
auto all_conversions = ArbitraryTemperatureConverter();
all_conversions.add([](double kelvin) { return kelvin; }, [](double kelvin) { return kelvin; }, {"Kelvin", "K", "k"});
all_conversions.add([](double celsius) { return celsius + 273.15; }, [](double kelvin) { return kelvin - 273.15; }, {"Celsius", "°C", "C", "c"});
all_conversions.add([](double delisle) { return 373.15 - delisle * 2 / 3; }, [](double kelvin) {return (373.15 - kelvin) * 3 / 2; }, {"Delisle", "°De", "De", "DE", "de"});
// etc.
for(int i = 3; i < argc; i++)
cout << all_conversions.convert(stod(argv[i]), argv[1], argv[2]) << endl;
return EXIT_SUCCESS;
}
And one more thing ...
Looking at the final code, I realized that the TemperatureConverter can be made simpler. Since there are only public members, this is essentially a struct. Plus, the constructor is trivial.
#include <iostream>
#include <map>
#include <string>
#include <vector>
#include <functional>
using namespace std;
struct TemperatureConverter {
std::function<double(double)> toKelvin;
std::function<double(double)> fromKelvin;
};
class ArbitraryTemperatureConverter
{
public:
void add(std::function<double(double)> toKelvin,
std::function<double(double)> fromKelvin,
const vector<string>& names) {
const auto converter = TemperatureConverter{toKelvin, fromKelvin};
for(const auto& name : names) {
converters.emplace(name, converter);
}
}
double convert(double temperature, const std::string& fromTempName, const std::string& toTempName) const {
const auto& toKelvin = get(fromTempName).toKelvin;
const auto& fromKelvin = get(toTempName).fromKelvin;
return fromKelvin(toKelvin(temperature));
}
private:
map<string, TemperatureConverter> converters;
const TemperatureConverter& get(const string& name) const {
return converters.at(name);
}
};
int main(const int argc, const char* const argv[]) {
auto all_conversions = ArbitraryTemperatureConverter();
all_conversions.add([](double kelvin) { return kelvin; }, [](double kelvin) { return kelvin; }, {"Kelvin", "K", "k"});
all_conversions.add([](double celsius) { return celsius + 273.15; }, [](double kelvin) { return kelvin - 273.15; }, {"Celsius", "°C", "C", "c"});
all_conversions.add([](double delisle) { return 373.15 - delisle * 2 / 3; }, [](double kelvin) {return (373.15 - kelvin) * 3 / 2; }, {"Delisle", "°De", "De", "DE", "de"});
// etc.
for(int i = 3; i < argc; i++)
cout << all_conversions.convert(stod(argv[i]), argv[1], argv[2]) << endl;
return EXIT_SUCCESS;
}
