8
\$\begingroup\$

I am curious if it would ever be in good practice to omit constructor definitions from a class. Here, the intention of the Temperature class is to simply convert between one temperature scale to another. Since there are three scale measurements, kelvin, Fahrenheit, and Celsius, I decided it was best to define specific functions for the desired conversion. All of the functions take a single argument of type double. This leaves me to think it's best to delete the constructors I defined. Also, any other feedback is welcomed too.

/*
    Create a Temperature class that returns a conversion to a user    
    selected scale
       Kelvin = Celsius + 273.15
       Celsius = (5.0/9) * (Fahrenheit - 32)
*/
#include <iostream>

class Temperature
{
    public:
        //default constructor
        Temperature() : kelvin(NULL), fahrenheit(NULL), celsius(NULL)
            {/*body intentionally left blank*/}

        //constructor: Kelvin, Fahrenheit, Celsius
        Temperature(double kel, double fahr, double cel) 
            : kelvin(kel), fahrenheit(fahr), celsius(cel)
            {/*body intentionally left blank*/}

        double kelToCel(double);
        double kelToFahr(double);
        double celToKel(double);
        double celToFahr(double);
        double fahrToCel(double);
        double fahrToKel(double);

    private:
        double kelvin = 0;
        double fahrenheit = 0;
        double celsius = 0;
};
double Temperature::kelToCel(double kel)
{
    kelvin = kel;
    return celsius = kel - 273.15;
}
double Temperature::kelToFahr(double kel)
{
    kelvin = kel;
    return fahrenheit = (9.0 / 5) * (kel - 273.15) + 32;
}
double Temperature::celToKel(double cel)
{
    celsius = cel;
    return kelvin = cel + 273.15;
}
double Temperature::celToFahr(double cel)
{
    celsius = cel;
    return fahrenheit = cel * (9.0 / 5) + 32;
}
double Temperature::fahrToCel(double fahr)
{
    fahrenheit = fahr;
    return celsius  = (5.0 / 9) * (fahr - 32);
}
double Temperature::fahrToKel(double fahr)
{
    fahrenheit = fahr;
    return  kelvin = (5.0 / 9) * (fahr - 32) + 273.15;
}
int main()
{
    std::cout << "This program converts between temperature scales.\n\n"
        << "1: Kelvin to Celsius       3: Fahrenheit to Kelvin     5:Celsius to Fahrenheit\n"
        << "2: Kelvin to Fahrenheit    4: Fahrenheit to Celsius    6: Celsius to Kelvin\n\n"
        << "Enter the number of your selection: ";

    short selection;
    std::cin >> selection;

    Temperature temp;
    double degrees, count = 0;

    switch (selection)
    {
       // input: Kelvin, output: Celsius
       case 1:
          std::cout << "Enter degrees in Kelvin: ";
          std::cin >> degrees;
          std::cout << temp.kelToCel(degrees) << " Celsius.\n";
          break;

       // input Kelvin, output: Fahrenheit
       case 2:
          std::cout << "Enter degrees in Kelvin: ";
          std::cin >> degrees;
          std::cout << temp.kelToFahr(degrees) << " Fahrenheit.\n";
          break;

       // input: Fahrenheit, output: Kelvin
       case 3:
          std::cout << "Enter degrees in Fahrenheit: ";
          std::cin >> degrees;
          std::cout << temp.fahrToKel(degrees) << " Kelvin.\n";
          break;

       // input: Fahrenheit, output Celsius
       case 4:
          std::cout << "Enter degrees in Fahrenheit: ";
          std::cin >> degrees;
          std::cout << temp.fahrToCel(degrees) << " Celsius.\n";
          break;

       // input: Celsius, output: Fahrenheit
       case 5:
          std::cout << "Enter degrees in Celsius: ";
          std::cin >> degrees;
          std::cout << temp.celToFahr(degrees) << " Fahrenheit.\n";
          break;

       // input: Celsius, output Kelvin
       case 6:
          std::cout << "Enter degrees in Celsius: ";
          std::cin >> degrees;
          std::cout << temp.celToKel(degrees) << " Kelvin\n";
          break;
       }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Just a little pedantic note - temperatures in Kelvin are not marked as "degrees". Wikipedia says: "Unlike the degree Fahrenheit and degree Celsius, the kelvin is not referred to or typeset as a degree." \$\endgroup\$ – user1118321 May 24 '15 at 14:40
  • \$\begingroup\$ Also, if you're planning on using this class for anything more than doing conversions, you could check the user's locale to decide how to display (Fahrenheit or Celsius). \$\endgroup\$ – user1118321 May 24 '15 at 14:46
  • 3
    \$\begingroup\$ Please do not make edits to the code after reviews have been posted, as it may void them; therefore I have rolled back your edit. You can visit our meta page to figure out how to handle this properly. \$\endgroup\$ – syb0rg May 24 '15 at 15:53
  • \$\begingroup\$ Mentally downvoting because 0 K = -273.16 C, not -273.15 C. \$\endgroup\$ – a CVn May 25 '15 at 8:10
  • 2
    \$\begingroup\$ @MichaelKjörling - You're contradicting your own source. Subtracting 273.16 K from the temperature of the triple point of water (0.01 °C) makes absolute zero (0 K) equivalent to −273.15 °C. It's −273.15 °C. \$\endgroup\$ – Davor May 25 '15 at 8:37
11
\$\begingroup\$

Rule of zero

Addressing your first question about destructors: the general rule to follow is that you hardly ever need to hand-code copy/move constructors, copy/move assignment operators and destructors, unless your class manages resources. That's called the Rule of Zero and it helps to easily write classes. Basically, there are two kinds of classes: the ones that manage resources and only do that (a class shouldn't try to do too many things, it should focus on doing one thing, that's the single responsibility principle) and the ones that don't. Most of the classes we write don't manage resources and in this case, it's better to let the compiler generate the special functions.

Design

I have the feeling that your class will be hard to use and maintain in a project: you are storing three different representations for a temperature and the user has to know which was used before using any the methods. That's some strong cognitive burden for users.

Ideally, you should have had a class Temperature which does not expose its representation. You should be able to tell to the class which degrees you are storing and which you want to retrieve. Basically, it would be something like that:

class Temperature
{
    public:

        void set_kelvin(double val);
        void set_celsius(double val);
        void set_fahrenheit(double val);

        double as_kelvin() const;
        double as_celsius() const;
        double as_fahrenheit() const;

    private:

        // Here, *you* choose how you store the temperature,
        // the user does not have to know about it
        double _degrees;
};

For example, you could always store Kelvin degrees and let the functions do the conversions (set_kelvin and as_kelvin wouldn't do more than assigning and returning the value). The user does not have to know how it is stored and what they have stored, only how to set and get the temperature the way they want.

Miscellaneous C++ things

  • Your default constructor uses NULL. Since you use in-class initializers, I guess that you at least use C++11. Therefore, you should use nullptr instead of NULL when you can. However, in your case, the use of NULL/nullptr is wrong: you're not initializing pointers anywhere, only double values. So what is happening is that NULL is interpreted as 0 then converted to the double value 0.0. What you should have done is use 0.0 directly, which is the correct value of the correct type.

  • That said, you could simply drop the default constructor. Actually, use = default to explicitly the compiler to generate it for you sinnce it you don't explicitly ask for it, it will be implicitly deleted because of the presence of other user-defined constructors:

    Temperature() = default;
    

    Since you have in-class initializers, the explicitly defaulted default constructor will use them to initialize the class members.

  • Your main function contains an unused count variable that you could delete. Your compiler should have warned you about it if you compile with a sufficient level of warnings.

\$\endgroup\$
  • \$\begingroup\$ I'd probably pick a constant multiplier which would allow stored values in all three scales to be retrieved exactly. As it is, as a double in kelvin, 0 °C is 273.149999999999977263 K, and 0 °F is 255.372222222222234222. Take a unit of 1/45 K and you get exactly representable double values: 12291.75 and 11491.75. The increment size per degree then becomes (obviously) 45 for C, and 25 for F. \$\endgroup\$ – Random832 May 24 '15 at 18:39
8
\$\begingroup\$

If you want to have this conversion as a class, I'd suggest following Morwenn's advice.

The posted implementation suggests a very different kind of implementation. The converters set but never access the data members in the class. The code would be shorter if you eliminated those data members. With no data members, those conversion functions could be static functions, as opposed to instance functions. But with no data, why use a class? A namespace works just as well.

Some other remarks:

  • Your functions have a lot of magic numbers in them. Best practice is to make those magic numbers defined constants of some sort.
  • The conversions are constant expressions, something that C++11 supports. Take advantage of this support.

With that, a non-class implementation follows.

namespace temperature
{
   constexpr double Zero_C_in_K = 273.15;
   constexpr double Zero_C_in_F = 32.0;
   constexpr double DegC_per_degF = 9.0 / 5.0;
   constexpr double DegF_per_degC = 1.0 / DegC_per_degF;

   constexpr double convert_K_to_C (double temp_in_K)
   {
      return temp_in_K - Zero_C_in_K;
   }

   constexpr double convert_C_to_K (double temp_in_C)
   {
      return temp_in_C + Zero_C_in_K;
   }

   constexpr double convert_C_to_F (double temp_in_C)
   {
      return temp_in_C*DegC_per_degF + Zero_C_in_F;
   }

   constexpr double convert_F_to_C (double temp_in_F)
   {
      return (temp_in_F - Zero_C_in_F) * DegF_per_degC;
   }

   constexpr double convert_K_to_F (double temp_in_K)
   {
      return convert_C_to_F(convert_K_to_C(temp_in_K));
   }

   constexpr double convert_F_to_K (double temp_in_F)
   {
      return convert_C_to_K(convert_F_to_C(temp_in_F));
   }
}

Some remarks on the above:

  • You might disagree with my naming conventions. I'm fine with that; people agree to disagree. I tried longer names for the conversion functions (e.g., convert_fahrenheit_to_celsius. That became a bit verbose with the last two functions. I've run into this before with scientific programming. There's never a clear-cut, this is the best way to do it. Using expressive names for units can get overly verbose when it comes to expressing things such as the Stefan-Boltzmann constant, 5.670373×10−8 watts per meter squared per kevin to the fourth power.
  • Regarding the last two functions: I chained previously defined conversion functions rather than writing yet another set of expressions. Because these are simple constexpr functions, a halfway decent compiler will optimize those internal calls away. This isn't so important here, but it can be very important when the number of different representations grows large. As an extreme, there are hundreds of ways to represent the way an object is oriented in three dimensional space. (The namespace approach wouldn't work with the representations of SO(3) problem.)
\$\endgroup\$
  • \$\begingroup\$ I disagree with your first remark. I don't think that constants you mention are magic number. They aren't going to change (do they?), their meaning is pretty obvious and they are basically used in one place. I see no point in introducing named constants. \$\endgroup\$ – el.pescado May 25 '15 at 18:03
  • \$\begingroup\$ @el.pescado - Pi and the number of degrees per radian are fixed, yet those are treated by many (most?) as "magic numbers". The speed of light is a defined physical constant, yet it too is treated by many (most?) as a magic number. \$\endgroup\$ – David Hammen May 26 '15 at 19:45
8
\$\begingroup\$

I would totally do a completely different design.

For each unit of measurement have a class that represents it. Then have constructors that take the other measurement types and convert them.

class F;  // You can type these out in full.
class C;
class K;

struct F
{
    int value;

        explicit F(int f):       value(f)  {}
        F(C const& c);
        F(K const& k);

        friend std::ostream& operator<<(std::ostream& str, F const& d) {
            return str << d.value;
        }
        friend std::istream& operator>>(std::istream& str, F& d) {
            return str >> d.value;
        }
};
class C
{   // Fill in
};
class K
{   // Fill in
};
// Define all the conversions here.
F::F(C const& c):  value(c.value * (9.0 / 5) + 32)            {}
F::F(K const& k):  value((k.value - 273.15) * (9.0 / 5) + 32) {}

Usage is now simple.

void func()
{
     std::cout << "Enter F value\n";
     F     fahrenheit;
     std::cin >> fahrenheit;

     // Convert to Celsius.
     C     celsius(fahrenheit);

     // Convert to Kelvin
     K     kelvin(fahrenheit);

     std::cout << "F: " << fahrenheit << "\n"
               << "C: " << celsius    << "\n"
               << "K: " << kelvin      << "\n";
}

The advantage here is that your temperature has both a type and a value. So the compiler will do the conversion for you when you pass tempratures around.

class HomeControl
{
    public:
        void setDefaultTemp(K const& k) {
            // Turn furnace on/off
        }
};

int main()
{
     HomeControl  home;
     F            fahrenheitRoomTemp(45);

     home.setDefaultTemp(fahrenheitRoomTemp);
     // Sets default temperature
     // using a Fahrenheit object even through an interface
     // that expects a kelvin value. The compiler sees
     // that there is a F -> K conversion and automatically
     // applies the conversion before calling the function.
}
\$\endgroup\$
7
\$\begingroup\$

The answer by @Morwenn already covers almost everything I was going to say, but in case you're using C++11 or above, you may also wish to consider user-defined literals. In particular, if you wished to use K as the internal storage unit, you might use something like this:

#include <iostream>

class Temperature
{
public:
    Temperature(long double val) : _degrees(val) {}
    void set_kelvin(double val) { _degrees = val; }
    void set_celsius(double val) { _degrees = CtoK(val); }
    void set_fahrenheit(double val) { _degrees = FtoK(val); }

    double as_kelvin() const { return _degrees; }
    double as_celsius() const { return KtoC(_degrees); }
    double as_fahrenheit() const { return KtoF(_degrees); }

    static constexpr long double KtoC(long double x) { return x-273.15; }
    static constexpr long double KtoF(long double x) { return x*9.0/5.0 - 459.67; }
    static constexpr long double CtoK(long double x) { return x+273.15; }
    static constexpr long double FtoK(long double x) { return (x+459.67)*5.0/9.0; }

private:

    // all temperatures are internally stored as Kelvin
    double _degrees;
};


constexpr long double operator"" _c(long double deg) {
    return Temperature::CtoK(deg);
}

constexpr long double operator"" _f(long double deg) {
    return Temperature::FtoK(deg);
}

constexpr long double operator"" _k(long double deg) {
    return deg;
}

int main()
{
    Temperature t1{78.0_f};
    std::cout << "t1 = " << t1.as_kelvin() << "K"
        " = " << t1.as_celsius() << " degrees C" <<  
        " = " << t1.as_fahrenheit() << " degrees F" << std::endl;
    t1.set_celsius(100.0);
    std::cout << "water boils at " << t1.as_kelvin() << "K"
        " = " << t1.as_celsius() << " degrees C" <<  
        " = " << t1.as_fahrenheit() << " degrees F" << std::endl;
}

Note that all of the conversions are implemented as static constexpr functions. This allows the implementation of the actual conversion exactly once for both internal Temperature and external uses, and incurs no runtime penalty for constants such as 78.0_f or 100.0_c.

Sample output

t1 = 298.706K = 25.5556 degrees C = 78 degrees F
water boils at 373.15K = 100 degrees C = 212 degrees F

A tidy approach

Assuming that all temperature scales are linear, one can generically infer that there will always be some conversion from one scale to the other using no more than a simple linear conversion. One way to do that would be to have the entire class data-driven and to have each scale defined solely by the points at which water freezes and at which water boils (at standard pressure). Here is such an approach:

#include <iostream>

class Temperature
{
public:
    enum Scale { CELSIUS, KELVIN, FAHRENHEIT };
    Temperature(long double val) : _degrees(val) {}
    void set_kelvin(double val) { _degrees = val; }
    void set_celsius(double val) { _degrees = convert(val, CELSIUS, KELVIN); }
    void set_fahrenheit(double val) { _degrees = convert(val, FAHRENHEIT, KELVIN); }

    double as_kelvin() const { return _degrees; }
    double as_celsius() const { return convert(_degrees, KELVIN, CELSIUS); }
    double as_fahrenheit() const { return convert(_degrees, KELVIN, FAHRENHEIT); }

    static constexpr long double convert(long double x, Scale from, Scale to) { 
        return (x - scales[from].waterfreeze)*
            (scales[to].waterboil - scales[to].waterfreeze)/
            (scales[from].waterboil - scales[from].waterfreeze)
            + scales[to].waterfreeze;
    }
private:
    struct TempScale {
        long double waterfreeze;
        long double waterboil;
        const char *name;
    };
    static constexpr TempScale scales[3]{ 
        { 0, 100, "C" },
        { 273.15, 373.15, "K" },
        { 32, 212, "F" },
    };

    // all temperatures are internally stored as Kelvin
    double _degrees;
};

constexpr Temperature::TempScale Temperature::scales[];

constexpr long double operator"" _c(long double deg) {
    return Temperature::convert(deg, Temperature::CELSIUS, Temperature::KELVIN);
}

constexpr long double operator"" _f(long double deg) {
    return Temperature::convert(deg, Temperature::FAHRENHEIT, Temperature::KELVIN);
}

constexpr long double operator"" _k(long double deg) {
    return deg;
}

int main()
{
    Temperature t1{20.0_c};
    std::cout << "t1 = " << t1.as_kelvin() << "K"
        " = " << t1.as_celsius() << " degrees C" <<  
        " = " << t1.as_fahrenheit() << " degrees F" << std::endl;
    t1.set_celsius(100.0);
    std::cout << "water boils at " << t1.as_kelvin() << "K"
        " = " << t1.as_celsius() << " degrees C" <<  
        " = " << t1.as_fahrenheit() << " degrees F" << std::endl;
    t1.set_celsius(0.0);
    std::cout << "water freezes at " << t1.as_kelvin() << "K"
        " = " << t1.as_celsius() << " degrees C" <<  
        " = " << t1.as_fahrenheit() << " degrees F" << std::endl;
}
\$\endgroup\$
  • \$\begingroup\$ What type of temperature does your construction take? It is difficult to see when you call it. Surely better to include Scale \$\endgroup\$ – Mark May 25 '15 at 9:36
  • \$\begingroup\$ As the code comment says, all temperatures are internally stored as Kelvin, which is what the constructor takes. The point to using the user-defined literals is to avoid having to worry about that. One could write Temperature t{68.0_F} or Temperature t{20.0_C} or Temperature t{293.15_K}. \$\endgroup\$ – Edward May 25 '15 at 11:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.