Temperature unit conversion (C,F,K) in C++ - follow-up

Question

Link to my first question:

Converting Celsius and Fahrenheit in C++

I've improved my code, following @Jamal's recommendations. I hope I did everything right. The only thing I didn't get was putting that tagline cout in a comment.

I know I could have used if instead of switch, but I was wondering if I can do it with switches.

The previous code had only 1 input for temperature, so it was easy. Here, since we have 3 units, we need either 2 inputs (which I used) or maybe it could have been 1 input as (cf - Celsius to Fahrenheit) for example, but since switch doesn't accept strings and I've decided to use switch, it was a bit difficult.

So, how does it look? Any suggestions?

#include <iostream>
#include <string>
using std::cout;
using std::cin;

char temperature; // first temperature unit
char temperature2; // second temperature unit
double value; // value that is being converted
int x,y; // used to check what function we will be using for output

void welcome () { // intro and first 2 inputs manual
    std::cout << "Welcome" << "\n" << "This little program (Version 2.0) converts the following temperature units: Celsius, Fahrenheit and Kelvins\nEnter first temperature unit which you want to convert and then a second one\nUse: c(C) for Celsius, f(F) for Fahrenheit or k(K) for Kelvins\nExample:\nc\nf\n";
    std::cin >> temperature >> temperature2; // storing what 2 temperature units are being converted, which accepts  c, C, f, F, k, K.
}

double switch2() {
    switch (temperature) { // checking input for one of 3 accepted characters, else using default
        case 'c' :
        case 'C' : {
            x = 1;
        }
        break;

        case 'f' :
        case 'F' : {
            x = 2;
        }
        break;

        case 'k' :
        case 'K' : {
            x = 3;
        }
        break;

        default:
            std::cout << "Invalid character " << temperature << "! Use c(C), f(F), k(K) for first temperature\n"; // any input except the 3 accepted options will result in this default output
    }
    return x;
}

double switch3() {
    switch (temperature2) { // checking input for one of 3 accepted characters, else using default
        case 'c' :
        case 'C' : {
            y = 1;
        }
        break;

        case 'f' :
        case 'F' : {
            y = 2;
        }
        break;

        case 'k' :
        case 'K' : {
           y = 3;
        }
        break;

        default:
            std::cout << "Invalid character " << temperature2 << "! Use c(C), f(F), k(K) for second temperature\n"; // any input except the 3 accepted options will result in this default output
    }
    return y;
}

// taking value and converting it depending on temperature units selected
void CtF (double value){
    std::cout << "Converting Celsius to Fahrenheit\n" << "Enter value:\n";
    std::cin >> value;
    double fahrenheit = value*(9.0/5.0)+32;
    std::cout << value << " Celsius in Fahrenheit is " << fahrenheit;
}

void CtK (double value){
    std::cout << "Converting Celsius to Kelvins\n" << "Enter value:\n";
    std::cin >> value;
    double kelvins = value + 273.15;
    std::cout << value << " Celsius in Kelvins is " << kelvins;
}

void FtC (double value){
    std::cout << "Converting Fahrenheit to Celsius" << "\n" << "Enter value:\n";
    std::cin >> value;
    double celsius = (value-32)/(9.0/5.0);
    std::cout << value << " Fahrenheit in Celsius is " << celsius;
}

void FtK (double value){
    std::cout << "Converting Fahrenheit to Kelvins" << "\n" << "Enter value:\n";
    std::cin >> value;
    double kelvins = (value + 459.67)*5.0/9.0;
    std::cout << value << " Fahrenheit in Kelvins is " << kelvins;
}

void KtC (double value){
    std::cout << "Converting Kelvins to Celsius" << "\n" << "Enter value:\n";
    std::cin >> value;
    double celsius = value - 273.15 ;
    std::cout << value << " Kelvins in Celsius is " << celsius;
}

void KtF (double value){
    std::cout << "Converting Kelvins to Fahrenheit" << "\n" << "Enter value:\n";
    std::cin >> value;
    double fahrenheit = value*(9.0/5.0) - 459.67 ;
    std::cout << value << " Kelvins in Fahrenheit is " << fahrenheit;
}

void result () { // function that executes 2 other functions and checks what output function to use
    switch2();
    switch3();
    if (x==1 && y==2){
        CtF(value);
    }
    if (x==1 && y==3){
        CtK(value);
    }
    if (x==2 && y==1){
        FtC(value);
    }
    if (x==2 && y==3){
        FtK(value);
    }
    if (x==3 && y==1){
        KtC(value);
    }
    if (x==3 && y==2){
        KtF(value);
    }
    if ((x==1 && y==1) || (x==2 && y==2) || (x==3 && y==3)) { // error handling
        std::cout << "Very funny :) \nYou can't convert the same temperature!";
    }
    std::cout << "\nMade by blackborg\n02.04.2018";
}

int main()
{
    welcome();
    result();
}

Answer 1

I'll go ahead and provide a quick write-up of a simpler version. I didn't include anything "fancy" from C++11 or above. I'm sure this can be even simpler or better, but this is just a start. I left out the extra outputs and input validation, but you already know how to add all of that. I also didn't bother too much with the naming here, but you get the idea.

To give the user less of a hassle, I just output both conversions with a given unit. You don't have to take this approach, but I thought it was a bit nicer.

#include <iostream>

struct Temperature
{
    double value;
    char unit;
};

const double calcCelsiusToFahrenheit(double celsius)
{
    return celsius * (9.0 / 5.0) + 32;
}

const double calcFahrenheitToCelsius(double fahrenheit)
{
    return fahrenheit - 32 * (5.0 / 9.0);
}

const double calcFahrenheitToKelvins(double fahrenheit)
{
    return fahrenheit + 459.67 * (5.0 / 9.0);
}

const double calcCelsiusToKelvins(double celsius)
{
    return celsius + 273.15;
}

const double calcKelvinsToCelsius(double kelvins)
{
    return kelvins - 273.15;
}

const double calcKelvinsToFahrenheit(double kelvins)
{
    return kelvins - 459.67;
}

const Temperature getTemperature()
{
    Temperature temp;

    std::cout << "Temperature: "
    std::cin >> temp.value;

    std::cout << "\nUnit: ";
    std::cin >> temp.unit;

    return temp;
}

void displayOtherTemps(Temperature temp)
{
    switch(std::toupper(temp.unit))
    {
    case 'C':
        std::cout << "\n\nFahrenheit: " << calcCelsiusToFahrenheit(temp.value);
        std::cout << "\nKelvins: " << calcCelsiusToKelvins(temp.value);
        break;

    case 'F':
        std::cout << "\n\nCelsius: " << calcFahrenheitToCelsius(temp.value);
        std::cout << "\nKelvins: " << calcFahrenheitToKelvins(temp.value);
        break;

    case 'K':
        std::cout << "\n\nFahrenheit: " << calcKelvinsToFahrenheit(temp.value);
        std::cout << "\nCelsius: " << calcKelvinsToCelsius(temp.value);
        break;
    }
}

int main()
{
    displayOtherTemps(getTemperature());
}

Notice what I did here:

• Simple main()

  All it does is call functions to get the results. You could probably still have it take the input, get the results from the functions, then print the results, but this also works.

• One purpose for each function

  Besides main(), each function has its own role:

  • The calcX functions return a single calculation
  • getTemperature gets the values from the user
  • displayResults takes the values and prints the calculations

  Your calculation functions, for instance, are doing three different things.

• Using a structure for an entity

  A temperature, in this case, can be thought to consist of two things: its value and its unit. Thus, instead of having separate variables to move around or have as global where they can be modified anywhere, you can put them in a struct to better convey their meaning.

• No "magic numbers"

  There are no 1, 2, or 3 in this code to get the code to "flow" in some direction, while confusing the reader. The numbers used in the calculations are an exception, though.

• std::toupper for the switch

  I've added std::toupper here to avoid the extra cases (I know I didn't mention this in the previous review, but I just remembered about it, so do consider this now).

Regardless of the approach you take, this should give you an idea of how you should simplify your code and make it flow better.

Answer 2

1. Don't use using std::[cout|cin]. As with using namespace std, it doesn't really help you a lot, and can, in certain circumstances, be harmful. Also, you are already writing std::[cout|cin] consistently, so those two declarations add nothing at all here.
2. Don't use global variables. They are indicative of bad design. In fact, you should probably write a class instead of some methods to carry state through your function calls.
3. You should really work on your function naming. switch2, switch3 etc. don't help much in grasping what that functions actually do. Even though it is difficult, try to always come up with useful names.
4. Your code does not observe the Single Responsibility Principle, which will inevitably lead to ununtangleable spaghetti code. The Single Responsibility Principle states that every logical unit in your piece of code (function, method, class, etc.) should have exactly on task. As of right now, you are mixing output with calculations and other program logic; extracting and clearly separating these things will be important for keeping code clean and maintainable in larger projects.
5. switch2 and switch3 both return a double which is never used. This, again, is indicating that you have one or more design issues. Think about whether it makes more sense to set some value outside the method (be it global or inside an object) or return it to the caller directly when you refactor this code.
6. You are not using anything from string, so you can safely remove the include for it.

Answer 3

I don’t think I’m supposed to write this for you, but when you start learning object-oriented programming, consider this approach, at least as an exercise:

There’s a class temperature, whose constructor accepts temperatures in any of several systems (the constructor takes value and system parameters) and stores it in one standard format, perhaps the private class variable double temperature_in_k;.

It has three member functions, celsius(), fahrenheit() and kelvin(). With no parameter, this is a const function that converts the temperature stored in the class to the requested scale. With a double parameter, it sets the temperature to that value in that scale. (But how should it handle t.kelvin(-1.0)? What should it return?)

For this trivial application, it doesn’t have a lot of advantages over just keeping the value itself in a scalar, and it sometimes means an unnecessary round-trip conversion, but it does separate all the quadratic combinations of formats into one conversion to the standard format and one conversion from the standard format, and gives a single component of the program a single responsibility for each.

PS

“Hungarian notation” was originally supposed to write the variable I named temperature_in_k askTemperature, not dTemperature. It was supposed to be annotating the meaning of the variable so you’ll notice that you’re passing a double temperature in Celsius to a function that takes a double temperature in Kelvin, which the compiler cannot check for you. But what it came to mean was just abbreviating the type of each variable, which the compiler already checks better than you can. Supposedly, the programmers writing closed-source applications at Microsoft did the former, but the programmers writing the APIs did the latter and gave the whole thing a bad name. You’ll also see a lot of code that writes it as kt, m_t or even just t. But at least add a short comment about what that variable represents and what constraints it has, if the name doesn’t make that obvious.

Update

Based on a comment, I decided to go ahead and write a partial example of what an implementation with different kelvin, celsius and faherenheit classes might look like.

It uses some fairly-advanced concepts, but for the moment you can just ignore stuff like constexpr and noexcept.

#include <new>

/* The interface of a generic temperature, which can be converted to an
 * absolute temperature in Kelvin, or its own native scale.
 */
class temperature {
  public:
    constexpr temperature() {} // To make the type literal.
/* Virtually all destructors should be vitrual, but in this case, all derived
 * classes are trivially-destructible and we want temperatures to have literal
 * type.
 */
    ~temperature() = default;

    // Convert to and from absolute temperature:
    virtual double absolute() const noexcept = 0;
    virtual double absolute(double) = 0;

    // Get and set the temperature in native units:
    virtual double native() const noexcept = 0;
    virtual double native(double) = 0;

/* There are no other arithmetic functions or conversion to double in the
 * generic interface, for the following reasons:
 *
 * The return type of assigning to a T should be T&, so each daughter class
 * needs to overload operator=(double) itself.
 *
 * It makes no sense to add a scalar like 1.0 to a temperature with unknown
 * units.  There is no physical reason to ever multiply or divide anything but
 * an absolute temperature to get another temperature, so this can be added to
 * kelvin if at all.  It never makes physical sense to flup the sign.
 *
 * Adding an implicit conversion to double would silently enable absurd, buggy
 * code such as -t, deg_c / 2.0, or in_unknown_units + 1.0, to work.
 */
};

class celsius : public temperature {
  public:
    //Rule of 3:

    constexpr celsius() : deg_c() {}
    // Throw an exception if x is not above absolute 0?
    constexpr celsius(const double x) : deg_c(x) {}
    // Copying the same type of temperature should not convert.
    constexpr celsius(const celsius& x) = default;
    // Not constexpr, because it calls a virtual function.
    celsius(const temperature& x) noexcept
    {
      absolute(x.absolute());
    }

    celsius& operator=(const celsius& x) = default;
    celsius& operator=(const temperature& x) noexcept
    {
      if (this == &x) {
        celsius::~celsius(); // Trivially destructible.
        new(this) celsius(x); // Don't repeat yourself.
      }

      return *this;
    }

    // Implementations of inherited virtual methods:

    double absolute() const noexcept
    {
      return deg_c + freezing_point_k;
    }

    // A real-world implementation would check the domain of x.
    double absolute(const double x)
    {
      return ((deg_c = x - freezing_point_k));
    }

    double native() const noexcept { return deg_c; }
    double native(const double x) { return ((deg_c = x)); }

/* Some mathematical operations still potentially make sense, particularly if
 * this class can also represent a temperature difference.
 */

  private:
    static constexpr double freezing_point_k = 273.15;
    double deg_c;
};

class fahrenheit : public temperature {
  public:
    //Rule of 3:

    constexpr fahrenheit() : deg_f() {}
    // Throw an exception if x is not above absolute 0?
    constexpr fahrenheit(const double x) : deg_f(x) {}
    // Copying the same type of temperature should not convert.
    constexpr fahrenheit(const fahrenheit& f) = default;
    // Not constexpr, because it calls a virtual function:
    fahrenheit(const temperature& x) noexcept
    {
      absolute(x.absolute());
    }

    fahrenheit& operator=(const fahrenheit& x) = default;
    fahrenheit& operator=(const temperature& x) noexcept
    {
      if (this == &x) {
        fahrenheit::~fahrenheit(); // Trivially destructible.
        new(this) fahrenheit(x); // Don't repeat yourself.
      }

      return *this;
    }

    // Implementations of inherited virtual methods:

    double absolute() const noexcept
    {
      return (deg_f - abs0_f) / f_per_k;
    }

    // A real-world implementation would check the domain of x.
    double absolute(const double x)
    {
      return ((deg_f = x*f_per_k + abs0_f )); // FIXME!
    }

    double native() const noexcept { return deg_f; }
    double native(const double x) { return ((deg_f = x)); }

/* Some mathematical operations still potentially make sense, particularly if
 * this class can also represent a temperature difference or we add Rankine.
 */

  private:
    static constexpr double abs0_f = -459.67;
    static constexpr double f_per_k = 9.0/5.0;
    double deg_f;
};

/* A class representing absolute temperature in Kelvin.  This has the simplest
 * possible implementation of the interface, but would have the largest number
 * of operations, since we only do most mathematical operations on absolute
 * temperatures.
 */
class kelvin : public temperature {
  public:
    //Rule of 3:

    constexpr kelvin() : above_abs0() {}
    // Throw an exception if x is not a non-negative real number?
    constexpr kelvin(const double x) : above_abs0(x) {}
    // Copying the same type of temperature should not convert.
    constexpr kelvin(const kelvin& x) = default;
    // Not constexpr, because it calls a virtual function.
    kelvin(const temperature& x) noexcept : above_abs0(x.absolute()) {}

    kelvin& operator=(const kelvin& x) = default;
    kelvin& operator=(const temperature& x) noexcept
    {
      if (this == &x) {
        kelvin::~kelvin(); // Trivially destructible.
        new(this) kelvin(x); // Don't repeat yourself.
      }

      return *this;
    }

    // Implementations of inherited virtual methods:

    double absolute() const noexcept { return above_abs0; }
    // A real-world implementation would check the domain of x.
    double absolute(const double x) { return ((above_abs0 = x)); }

    double native() const noexcept { return absolute(); }
    double native(const double x) { return absolute(x); }

/* It potentially makes sense to enable expressions for absolute temperatures
 * such as: thermal_energy = heat_capacity*t; t1 = t0+delta_t; pv = (n*r)/t;
 *          delta_t = t1-t0;
 *
 * You would do this by overloading those operators, but not unary minus.
 */

  private:
    double above_abs0;
};

// A simplistic test Driver without full code coverage:

#include <iostream>
#include <stdlib.h>

using std::cout; // Take that!

int main()
{
  constexpr celsius freezing_point_c = 0;
  constexpr fahrenheit zero_f = 0;
  constexpr kelvin absolute_zero_k = 0;
  const kelvin boiling_point_k = celsius(100);

  cout << "0 C is " << fahrenheit(freezing_point_c).native() << " F and "
       << kelvin(freezing_point_c).native() << " K.\n";
  cout << "Absolute zero is " << celsius(absolute_zero_k).native() << " C and "
       << fahrenheit(absolute_zero_k).native() << " F.\n";;
  cout << "0 F is " << celsius(zero_f).native() << " C and " 
       << kelvin(zero_f).native() << " K.\n";
  cout << "The boiling point of water is " << celsius(boiling_point_k).native()
       << " c and " << fahrenheit(boiling_point_k).native() << " F.\n";

  return EXIT_SUCCESS;
}

A few things to notice about this implementation: a celsius object cannot be passed into a function that expects a kelvin, or vice versa; but one that accepts any temperature& can accept either. The only code that ever has to deal with any of the internal details of any temperature scale is encapsulated inside the class. You can easily overload the operators of derived classes to allow expressions such as t1 = t0 + delta_t; pv = (n*r)/t; or thermal_energy = t*heat_capacity; but not -t or temperature_in_unknown_units + 1.0.

If we wanted to introduce more specific conversions, such as constexpr constructors for fahrenheit and celsius that take the other as arguments, we can still do that through overrides and friend declarations.

Answer 4

First, you are getting the real work mixed up with the I/O of the testing. In real code, you’ll have to supply some functionality such as converting temperature, and it works by taking inputs and producing outputs. Some other part of the program will use that while driving the GUI, or in this case, a unit test program will call it. Rather than ask the user for the test cases (that gets old fast) it might read them from a file or have them compiled in to the tester.

So, I have here a function convert that does the work, and call it from a testing function.

From the top: I use a typedef instead of double everywhere. Besides not locked into double if this ever changes, the name documents which variables and parameters hold that. Imagine a function that takes 5 doubles and 3 ints — which is which? If it takes a temperature, a cost, a time, etc. then the positions are clearer, even in cases where the compiler doesn’t check it.

Your original code had n×n different code paths for every possible conversion. That’s not scalable. By knowing only how to convert whatever to/from kelvin, I only need n. You also have helper functions setting global variables — that’s bad. In your code you could have easily just returned the value from the function instead. I do that many times in the code below; that is, the helper function supplies a value which the caller saves in a named variable.

Rather than a separate function for each conversion, I see your code is repeating almost exactly the same thing each time. That is, the conversion factor should be a parameter to universal convert code.

So, I start by defining the scales in an array of structures. The rest of the code will be unchanged if I add more or otherwise change this! For example, I added Rankine and recompiled, and nothing needed to be fixed.

So, the convert function looks up the definitions for each scale, and uses the values it finds in those records.

Now the convert function, like everything else, uses an enumeration type for the different scale choices. That’s fine for hard-coded uses, but how do you read that from a file or populate a pull-down field? I supplied a find_scale function that takes an initial and returns the matching scale record. Note that it’s not a case statement (nor an if statement)! Instead, it’s a call to find_if passed over the array of scales it knows about. That’s why it understood 'R' without having to update the function!

(I don’t like how std::find_if requires separate begin/end parameters when I want to give it a whole collection. Boost’s range form is better here, but I stuck with std functions.)

The stuff to ship is at the top, and then there is a clear separator with the tester below. In real life, that would be a separate file, and the stuff would be packaged in a header + implementation files, and use namespaces. But you get the idea of separating.

The declaration using std::cout; is just fine inside the tester code, or in CPP files (not .h files) in general.

#include <iostream>
#include <algorithm>
#include <stdexcept>
#include <string>
#include <type_traits>

using temperature_value = double;  // used alone, represents kelvin.

enum class temperature_scale {
    kelvin = 0,
    Celsius, Fahrenheit, Rankine
    // others can be added.
};

struct temperature {  // express temperature unambiguously with units
    temperature_value v;
    temperature_scale scale;
};

struct temp_scale_info_t {
    temperature_scale scale;
    const char* name;
    // conversion is linear equn Y=mX+b, where input X is value in k
    // and output Y is value in this scale.
    double m, b;
};

constexpr temp_scale_info_t temp_scale_info[] = {
    { temperature_scale::kelvin, "kelvin", 1.0, 0.0 },
    { temperature_scale::Celsius, "Celsius", 1.0, -273.15 },
    { temperature_scale::Fahrenheit, "Fahrenheit", 1.8, -459.67 },
    { temperature_scale::Rankine, "Rankine", 1.8, 0.0 }
};

const temp_scale_info_t& find_scale (char ch)
{
    const int case_difference = 'a' - 'A';
    if (ch >= 'a' && ch <= 'z') ch = char(ch - case_difference);
    const char ch2 = char(ch + case_difference);
    auto it= std::find_if (std::begin(temp_scale_info), std::end(temp_scale_info),
        [=](const auto& rec) { char initial=rec.name[0]; return initial==ch || initial==ch2; }
    );
    if (it == std::end(temp_scale_info))  throw std::invalid_argument("no such scale");
    return *it;
}

const temp_scale_info_t& get_info (temperature_scale scale)
{
    size_t index= size_t(scale);
    if (index >= std::extent_v<decltype(temp_scale_info)>)  throw std::invalid_argument("no such scale");
    const auto& rec= temp_scale_info[index];
    if (rec.scale != scale)  throw std::logic_error("temp_scale_info table set up incorrectly");
    return rec;
}

temperature convert (temperature t_in, temperature_scale wanted)
{
    if (t_in.scale == wanted)  return t_in;
    const auto& recin= get_info (t_in.scale);
    // convert input to k
    // Y = mX + b, so X = (Y-b)/m
    temperature_value val_in_k = (t_in.v-recin.b)/recin.m;
    // convert to desired output
    const auto& recout= get_info (wanted);
    return { recout.m*val_in_k + recout.b,  wanted };
}

const char* name (temperature_scale scale)
{
    const auto& rec= get_info(scale);
    return rec.name;
}

std::string format (temperature t)
{
    std::string result = std::to_string(t.v);
    result.push_back (' ');
    if (t.scale != temperature_scale::kelvin) result.append ("degrees ");
    result.append(name(t.scale));
    return result;
}

// ==============================

using std::cout;

void testit (temperature t_in, temperature_scale wanted)
{
    temperature t_out = convert (t_in, wanted);
    cout << format(t_in) << " is " << format(t_out) << '\n';
}

void run_tests()
{
    testit ({ 0,temperature_scale::Celsius }, temperature_scale::Fahrenheit);
    testit ({ 0,temperature_scale::Celsius }, temperature_scale::kelvin);
    testit ({ 0,temperature_scale::kelvin }, temperature_scale::Celsius);
    testit ({ 100,temperature_scale::Celsius }, temperature_scale::Fahrenheit);
    testit ({ -65,temperature_scale::Fahrenheit }, temperature_scale::Celsius);
    testit ({ -65,temperature_scale::Fahrenheit}, temperature_scale::kelvin);

    auto dest_scale = find_scale('R').scale;  // user input, file parsing, etc. will use this
    testit ({ -65,temperature_scale::Fahrenheit }, dest_scale);
}

int main()
{
    cout << "running\n";
    try {
        run_tests();
    }
    catch (std::exception& ex) {
        cout << "ERROR: exception " << ex.what() << '\n';
    }
}

Answer 5

switch2 and switch3 seem to do the same thing, other than they modify different global variables. Which is bad programming on several levels (repetition, using global variables, using functions for their side effects rather than return value). You seem to be using functions simply as a way to break your code into separate blocks.

((x==1 && y==1) || (x==2 && y==2) || (x==3 && y==3)) is an extremely wordy way of saying x==y, unless you want to exclude the possibility that x is not in [1,2,3].

Most of the code in the different conversions is the same. If you use variables for the strings representing original and final units, then all but the third line in the conversions can be made the same; those lines can then be pulled out of the functions and put in the main program.

The line

std::cout << "Welcome" << "\n" << "This little program (Version 2.0) converts the following temperature units: Celsius, Fahrenheit and Kelvins\nEnter first temperature unit which you want to convert and then a second one\nUse: c(C) for Celsius, f(F) for Fahrenheit or k(K) for Kelvins\nExample:\nc\nf\n";

is 308 characters. 308 characters is waaaaay too many for one line. If you post you code and side scrolling is needed, you should be looking at whether you can shorten your lines.