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

Celsius → Fahrenheit conversion table

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

Problem

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

#include 
#include 
#include 
#include 

#define DEGREE_SIGN "\u00B0"

class Fahrenheit;

class Celsius {
  public:
    explicit Celsius(double c) : c(c) {
        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 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 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?

Solution


  • 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&).



  • The Celsius(Fahrenheit) constructor should probably take its argument by const reference.



  • 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.



-
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.

  • operator+= should take its argument by const reference.



  • 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.



  • 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.



  • Fahrenheit::MIN_VALUE and Fahrenheit::MAX_VALUE should be calculated off the value of Celsius::MIN_VALUE and Celsius::MAX_VALUE.



  • All the comments about Celsius above apply to Fahrenheit too.



  • Use std::cerr to print out the usage message, not std::cout.



  • Use '\n' instead of std::endl. See The little endl that couldn't for rationale.



  • 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.



  • The elses in your error checks are superfluous since all of the preceding if branches are exiting conditions.



  • 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.

Code Snippets

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

Context

StackExchange Code Review Q#44836, answer score: 14

Revisions (0)

No revisions yet.