0
\$\begingroup\$

Problem statement:

Write a program that converts all given temperatures from a given input temperature scale to a given output temperature scale. The temperature scales to be supported are Kelvin, Celsius, Fahrenheit, Rankine, Delisle, Newton, Rømer, Réaumur.

Synopsis: tempconv INPUT_SCALE OUTPUT_SCALE [TEMPERATURE]...

The INPUT_SCALE and OUTPUT_SCALE shall be given as follows:

  • K for Kelvin
  • C for Celsius
  • F for Fahrenheit
  • R for Rankine
  • D for Delisle
  • N for Newton
  • for Rømer
  • for Réaumur.

Other, longer names should also be possible, like °C or Celsius for Celsius (see code).

Example:

tempconv K C 0 273.15 373.15
-273.15
0.0
100.0

My solution in C++:

#include <iostream>
#include <map>
#include <string>
#include <vector>

using namespace std;

class TemperatureConverter {
public:
    double (*toKelvin)(double);
    double (*fromKelvin)(double);

    static TemperatureConverter* get(string name) {
        return converters[name];
    }

    TemperatureConverter(
        double (*toKelvin)(double),
        double (*fromKelvin)(double),
        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 = {};
#define TEMPERATURE_CONVERTER(name, toKelvinExpression, fromKelvinExpression, ...) \
    TemperatureConverter name = TemperatureConverter( \
        [](double name) { return toKelvinExpression; }, \
        [](double kelvin) { return fromKelvinExpression; }, \
        {__VA_ARGS__} \
    )

TEMPERATURE_CONVERTER(kelvin,     kelvin,                           kelvin,                            "Kelvin", "K", "k");
TEMPERATURE_CONVERTER(celsius,    celsius + 273.15,                 kelvin - 273.15,                   "Celsius", "°C", "C", "c");
TEMPERATURE_CONVERTER(delisle,    373.15 - delisle * 2 / 3,         (373.15 - kelvin) * 3 / 2,         "Delisle", "°De", "De", "DE", "de");
TEMPERATURE_CONVERTER(fahrenheit, (fahrenheit + 459.67) * 5 / 9,    kelvin * 9 / 5 - 459.67,           "Fahrenheit", "°F", "F", "f");
TEMPERATURE_CONVERTER(newton,     newton * 100 / 33 + 273.15,       (kelvin - 273.15) * 33 / 100,      "Newton", "°N", "N", "n");
TEMPERATURE_CONVERTER(rankine,    rankine * 5 / 9,                  kelvin * 9 / 5,                    "Rankine", "°R", "R", "r");
TEMPERATURE_CONVERTER(réaumur,    réaumur * 5 / 4 + 273.15,         (kelvin - 273.15) * 4 / 5,         "Réaumur", "Reaumur", "°Ré", "°Re", "Ré", "RÉ", "ré", "Re", "RE", "re");
TEMPERATURE_CONVERTER(rømer,      (rømer - 7.5) * 40 / 21 + 273.15, (kelvin - 273.15) * 21 / 40 + 7.5, "Rømer", "Romer", "°Rø", "°Ro", "Rø", "RØ", "rø", "Ro", "RO", "ro");

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;
}

Note: There are a few feedbacks that I'm aware and that I'm not interested in:

  • The lines defining the converters are long. However, in this case, the length allows for a tabular source format, which in this case I find more readable.
  • using namespace std. This is a simple program. I don't use using namespace std in complex programs for known reasons. These reasons do not apply to simple programs like this.
  • There is no error handling for invalid command line arguments (insufficient count, unknown scales, strings that aren't numbers). I'm aware of that and I'm fine with that.

I'm looking for things that can be done smarter and more concise. For example, can the vector be simplified? Can the initialization be simplified? Is there a way to get a roughly equivalently concise implementation without a macro, like any shortcuts for lambda expressions that I'm not aware of?

\$\endgroup\$
5
  • 4
    \$\begingroup\$ It seems you're posting many similar questions in different languages. It's usually better not to post them all at once, as there's likely design issues that affect all the languages you're using, which you could use to make the subsequent programs even better before posting for review. \$\endgroup\$ Commented Sep 7, 2022 at 12:18
  • \$\begingroup\$ @TobySpeight I get that, and that is good advice. I have reasons to be confident that my solution does not have a design issue that transcends language, and expect most feedback to be about language-specifics. Of course, I'll be more than happy to be proven wrong about that, \$\endgroup\$ Commented Sep 7, 2022 at 12:23
  • 2
    \$\begingroup\$ I've looked at some of the others, and the ones I've looked at also seem to need a pair of mutually-consistent functions to convert to/from each unit, where a slope and offset are sufficient to define both conversions. So that's a design issue that's common to at least a few languages' implementations. \$\endgroup\$ Commented Sep 7, 2022 at 15:57
  • \$\begingroup\$ @TobySpeight Right. However, that's something that I wouldn't necesarily want to change. It's a coincidence of the chosen scales that they have a linear relationship. The Dalton scale is non-linear, for example. \$\endgroup\$ Commented Sep 7, 2022 at 17:12
  • 3
    \$\begingroup\$ @ChristianHujer All useful temperature scales are linear. According to PSE, the Dalton scale is a curiosity only. Toby's feedback, which aligns with mine, stands: the current solution over all languages is over-designed. \$\endgroup\$
    – Reinderien
    Commented Sep 7, 2022 at 22:11

2 Answers 2

5
\$\begingroup\$

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;
}
\$\endgroup\$
1
  • \$\begingroup\$ Thank you so much for the thorough review, really amazing! \$\endgroup\$ Commented Sep 7, 2022 at 17:05
3
\$\begingroup\$

You know that using namespace is harmful and that your error-handling is inadequate, so I'll leave you to fix those in your own time.

This code appears over-complex for the problem being solved. Some things in particular could be much simpler:

  • We need to write two functions for every temperature unit we add (forward and reverse conversions, that need to be made consistent with each other), yet each can be described with just two constants (slope and intercept).
  • The macro that creates variables (why not constants?) isn't really necessary. It's rare that we need to use macros in C++, and when we do, it's generally good to #undef as soon as reasonably possible when they are finished with, since macros transcend identifier scope.
  • The converters map retains pointers to objects after they go out of scope. In this program, that scope is sufficiently long, but this is a practice we should avoid.

I would propose something more like the following. We could encapsulate the list of converters a little more, perhaps, or construct it as a constexpr std::map in an immediately-evaluated lambda. We could also (more functional style) compose the in→out converter from the two Kelvin conversions. But it avoids the issues mentioned above, leaving just the i/o robustness to address:

#include <iostream>
#include <map>
#include <string>
#include <string_view>

struct conversion
{
    double per_kelvin;         // number of units in 1 kelvin interval
    double zero_val;           // value of zero, in kelvins

    double to_kelvins(double t) const noexcept
    {
        return t / per_kelvin + zero_val;
    }
    double from_kelvins(double t) const noexcept
    {
        return (t - zero_val) * per_kelvin;
    }
};


int main(int, char **argv)
{
    std::map<std::string, conversion> converters = {};

    auto add =
        [&converters](double per_kelvin, double zero_val,
                      std::initializer_list<std::string_view> names)
        {
            for (auto name: names) {
                converters.emplace(name, conversion{per_kelvin, zero_val});
            }
        };

    add(1,     0,      {"Kelvin", "Kelvins", "K", "k"});
    add(1,     273.15, {"Celsius", "°C", "C", "c"});
    add(-1.5,  373.15, {"Delisle", "°De", "De", "DE", "de"});
    add(1.8,   255.372,{"Fahrenheit", "°F", "F", "f"});
    add(1.8,   459.67 / 1.8 , {"Fahrenheit", "°F", "F", "f"});
    add(1,     273.15, {"Newton", "°N", "N", "n"});
    add(1.8,   0,      {"Rankine", "°R", "R", "r"});
    add(0.8,   273.15, {"Réaumur", "Reaumur", "°Ré", "°Re", "Ré",
                        "RÉ", "ré", "Re", "RE", "re"});
    add(0.525, 273.15 - 7.5 / 0.525,
                       {"Rømer", "Romer", "°Rø", "°Ro", "Rø", "RØ",
                        "rø", "Ro", "RO", "ro"});


    auto in_unit = converters.at(argv[1]);
    auto out_unit = converters.at(argv[2]);
    for (char **arg = argv+3;  *arg;  ++arg) {
        auto kelvins = in_unit.to_kelvins(atof(*arg));
        std::cout << out_unit.from_kelvins(kelvins) << '\n';
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Thank you so much for the thorough review, really amazing! \$\endgroup\$ Commented Sep 7, 2022 at 17:05

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.