Celsius → Fahrenheit conversion table

Question

It's harder to do than criticize. Here's my attempt to implement a Celsius-to-Fahrenheit conversion table in C++.

#include <cstdlib>
#include <cstring>
#include <iomanip>
#include <iostream>

#define DEGREE_SIGN "\u00B0"

class Fahrenheit;

class Celsius {
  public:
    explicit Celsius(double c) : c(c) {
        if (c < MIN_VALUE) {
            throw "Temperature below minimum";
        } else if (c > MAX_VALUE) {
            throw "Temperature above maximum";
        }
    }
    Celsius(Fahrenheit f);
    operator double() const { return c; }
    Celsius operator +(Celsius incr) const { return Celsius(c + incr.c); }
    Celsius &operator +=(Celsius incr) { c += incr.c; return *this; }
    friend std::ostream &operator<<(std::ostream &os, Celsius c) {
        return os << (double)c << DEGREE_SIGN << 'C';
    }

    static const Celsius MIN;  // Absolute zero
    static const Celsius MAX;  // Absolute hot (approx.)
  private:
    double c;
    static const double MIN_VALUE;
    static const double MAX_VALUE;
};


class Fahrenheit {
  public:
    explicit Fahrenheit(double f) : f(f) {
        if (f < MIN_VALUE) {
            throw "Temperature below minimum";
        } else if (f > MAX_VALUE) {
            throw "Temperature above maximum";
        }
    }
    Fahrenheit(Celsius c);
    operator double() const { return f; }
    Fahrenheit operator +(Fahrenheit incr) const { return Fahrenheit(f + incr.f); }
    Fahrenheit &operator +=(Fahrenheit incr) { f += incr.f; return *this; }
    friend std::ostream &operator<<(std::ostream &os, Fahrenheit f) {
        return os << (double)f << DEGREE_SIGN << 'F';
    }

    static const Fahrenheit MIN;  // Absolute zero
    static const Fahrenheit MAX;  // Absolute hot (approx.)
  private:
    double f;
    static const double MIN_VALUE;
    static const double MAX_VALUE;
};

Celsius::Celsius(Fahrenheit f) : c((f - 32) / 9 * 5) {}
const double Celsius::MIN_VALUE = -273.15;
const double Celsius::MAX_VALUE = 1.4e32;
const Celsius Celsius::MIN = Celsius(Celsius::MIN_VALUE);
const Celsius Celsius::MAX = Celsius(Celsius::MAX_VALUE);

Fahrenheit::Fahrenheit(Celsius c) : f(c / 5 * 9 + 32) {}
const double Fahrenheit::MIN_VALUE = -459.67;
const double Fahrenheit::MAX_VALUE = 2.5e32;
const Fahrenheit Fahrenheit::MIN = Fahrenheit(Fahrenheit::MIN_VALUE);
const Fahrenheit Fahrenheit::MAX = Fahrenheit(Fahrenheit::MAX_VALUE);

#ifdef _WIN32
const char PATH_SEPARATOR = '\\';
#else
const char PATH_SEPARATOR = '/';
#endif

const char *basename(const char *path) {
    const char * sep = strrchr(path, PATH_SEPARATOR);
    return sep ? sep + 1 : path;
}

int main(int argc, char *argv[]) {
    if (argc != 4) {
        std::cout << "Usage: " << basename(argv[0]) << " MIN MAX STEP" << std:: endl;
        return 1;
    }
    try {
        char *minEnd, *maxEnd, *stepEnd;
        Celsius min(strtod(argv[1], &minEnd)),
                max(strtod(argv[2], &maxEnd)),
                step(strtod(argv[3], &stepEnd));
        if (*minEnd != '\0') {
            std::cerr << "Invalid minimum temperature" << std::endl;
            return 1;
        } else if (*maxEnd != '\0') {
            std::cerr << "Invalid maximum temperature" << std::endl;
            return 1;
        } else if (*stepEnd != '\0') {
            std::cerr << "Invalid temperature step" << std::endl;
            return 1;
        }

        if (step <= 0.0) {
            std::cerr << "Step must be positive" << std::endl;
            return 1;
        } else if (step > max - min) {
            std::cerr << "Step exceeds temperature range" << std::endl;
            return 1;
        } else if (max < min) {
            std::cerr << "Minimum temperature exceeds maximum temperature" << std::endl;
            return 1;
        }
        for (Celsius c = min; c <= max; c += step) {
            std::cout << std::setw(8) << c
                      << std::setw(8) << Fahrenheit(c)
                      << std::endl;
        }
    } catch (const char *msg) {
        std::cerr << msg << std::endl;
        return 1;
    }
    return 0;
}

Some concerns I have:

• What do you think of the input validation?
• Is the code repetition justified? Much of the Fahrenheit class is not strictly necessary for the problem, and is included for completeness. Still, does the repetition enhance clarity, or is it harmful?

Answer 1

1. Don't throw string literals directly. Instead throw an instance of std::exception or (even better) a subclass thereof (in this case, I would choose either std::domain_error or std::out_of_range). It makes it easier for people to catch using catch (std::exception&).
2. The Celsius(Fahrenheit) constructor should probably take its argument by const reference.
3. I don't like the operator double conversion operator; it seems too easy to accidentally invoke. I think it's better to have a member function that you have to explicitly call to get its numeric value.

4. operator+ is usually better to be a free function. That way, you can use the standard implementation:

   Celsius operator+(Celsius lhs, Celsius const& rhs) {
       return lhs += rhs;
   }
   

   noting that lhs is passed by value. There are other reasons for having operator+ be a free function, such as being able to use other types for the LHS, other types for the RHS, etc., but here, the main advantage is that you can take the LHS by value.

5. operator+= should take its argument by const reference.
6. I would actually embed the degree sign straight into the string literal(s), rather than using a #define (the use of which is discouraged in C++). We live in a UTF-8 world.
7. Do not use C-style casts ((double)c as you had it). Here, it's much clearer to just write c.value() (if you added that member function as I suggested above; this has the added benefit of not requiring friend access) or c.c. In the general case of casting, it'd be better to use (in this case) a static_cast.
8. Fahrenheit::MIN_VALUE and Fahrenheit::MAX_VALUE should be calculated off the value of Celsius::MIN_VALUE and Celsius::MAX_VALUE.
9. All the comments about Celsius above apply to Fahrenheit too.
10. Use std::cerr to print out the usage message, not std::cout.
11. Use '\n' instead of std::endl. See The little endl that couldn't for rationale.
12. It's much more robust to parse numbers using boost::lexical_cast instead of strtod and then checking the end pointers. In particular, your code doesn't handle the case of empty strings correctly, and will treat them as valid.
13. The elses in your error checks are superfluous since all of the preceding if branches are exiting conditions.
14. Use EXIT_SUCCESS and EXIT_FAILURE for the exit codes instead of 0 and 1. Also, the bottom successful return is superfluous and should be removed.

To answer your bottom question, by YAGNI principles, much of your Fahrenheit class should be omitted. But if you want to be general, you could have written a templated TemperatureConverter, of which Celsius and Fahrenheit could both be specialised typedefs.

I also agree with the other two answers (also by Chrises, hah), and have upvoted them both.

Answer 2

I would have gone (and actually have gone in production code) for a Temperature class providing methods to obtain the values in the different units. This makes the streaming output slightly trickier but avoids the code repetition and makes it easier to lets say add Kelvin to it.

I'd also prefer to use C++11 in class member initializers for the min and max limits rather than initializing them outside (unavoidable though if you don't have a C++11 compiler at hand).

Also I don't like errors which say "This value is outside of the valid range" without telling me what the valid range actually is (which is not necessarily obvious if I don't have the source to check)

Answer 3

My first reaction was the same as ChrisWue's: consider using a single Temperature class instead. For example here is an example of a (C#) class handle angle units (i.e. radians and degrees).

Alternatively, the repetition/duplication between the two classes could perhaps be avoided with a common base class or by using templates.

Still, does the repetition enhance clarity, or is it harmful?

I had a few concerns:

• What a wall of text for such a simple problem! It reminded me of this joke.

• I had to inspect the core of it twice, for example:

  f(c / 5 * 9 + 32)
  

  and

  c((f - 32) / 9 * 5)
  

  No way around that; it's because you implemented functionality (bi-directional conversion) that wasn't required in the original problem (which only needed uni-directional conversion).

• Checking that you didn't have the same bug as the original solution required me to check two lines instead of just one, i.e.:

  f(c / 5 * 9 + 32)
  

  and:

  operator double() const { return c; }
  

• I was puzzled to find PATH_SEPARATOR in this code ("Why are you handling paths? Oh, I see.") If you have it, you needn't have defined it at global scope (it could have been inside basename which could have been inside main, or it could have been inside its own class.

• You have explicit construction from double, which is good. Perhaps it would be clearer to have explicit conversion to double too.

• operator+ can be defined using operator+= as discussed elsewhere.

What do you think of the input validation?

You throw a string instead of some exception class. Your main knows that it needs to catch a string, but if the classes were reused elsewhere it might be better to throw something else.

If you throw it might be helpful to the user to print not only the error message, but also the error value and which temperature (high or low) which failed to intialize.

Maybe you'd like to be able to count downwards (negative step).

I doubt that step should be of type Celcius and subject to the same min/max range validation as Celcius: by analogy, the difference (i.e. step) between two dates is not a date, but a time-span (e.g. a number of days or seconds).

Answer 4

For a strictly C++ effort, your use of objects could be considered commendable, from a certain perspective. However, your program is very long for a table. In fact, I wrote a similar program a few days ago that uses only 14 lines of code. To implement the degree sign would be 15, and most of it is just a simple for-loop. You would do well to avoid overcomplicating with objects if you want to do a task that requires some text output.

Instead of using a class, make a namespace that includes a double function that returns the calculated number. Take a look at lines 17 to 31 here.

You should also consider implementing getopt from unistd.h so that you don't have to write your own argv[] parser.