7
\$\begingroup\$

This is an implementation of a practice problem from cprogramming.com: Temperature Converter Challenge

The problem:

In this challenge, write a program that takes in three arguments, a start temperature (in Celsius), an end temperature (in Celsius) and a step size. Print out a table that goes from the start temperature to the end temperature, in steps of the step size; you do not actually need to print the final end temperature if the step size does not exactly match. You should perform input validation: do not accept start temperatures less than a lower limit (which your code should specify as a constant) or higher than an upper limit (which your code should also specify). You should not allow a step size greater than the difference in temperatures. (This exercise was based on a problem from C Programming Language).

The code:

#include <iostream>
#include <string>
#include <sstream>
#include <iomanip>

using namespace std;

const double LowestLimit = 0;
const double HighestLimit = 50000;
const string LowerLimitMessage = "Please enter a lower limit, limit >= 0: ";

struct UserInputs
{
public:
    double LowerLimit = -1;
    double UpperLimit = -1;
    double StepAmount = -1;
};

stringstream GetSingleInput(const string message)
{
    stringstream returnStream;
    string input;
    cout << message;
    cin >> input;
    returnStream << input;
    return returnStream;
}

UserInputs GetUserInputs()
{
    UserInputs parsedInputs;
    auto LowerLimitStream = GetSingleInput(LowerLimitMessage);

    while (!(LowerLimitStream >> parsedInputs.LowerLimit) || parsedInputs.LowerLimit < LowestLimit)
    {
        cout << "Invalid Lower Limit!\n";
        LowerLimitStream = GetSingleInput(LowerLimitMessage);;
    }

    stringstream UpperLimitMessage;
    UpperLimitMessage << "Please enter an upper limit, limit > "
        << parsedInputs.LowerLimit
        << ", limit <= "
        << HighestLimit
        << ": ";

    auto UpperLimitStream = GetSingleInput(UpperLimitMessage.str());

    while (!(UpperLimitStream >> parsedInputs.UpperLimit)
        || parsedInputs.UpperLimit > HighestLimit
        || parsedInputs.UpperLimit <= parsedInputs.LowerLimit)
    {
        cout << "Invalid Upper Limit!\n";
        UpperLimitStream = GetSingleInput(UpperLimitMessage.str());
    }

    stringstream stepMessage;
    stepMessage << "Please enter a step amount , step > "
        << LowestLimit
        << ", step <= "
        << (parsedInputs.UpperLimit - parsedInputs.LowerLimit)
        << ": ";


    auto stepStream = GetSingleInput(stepMessage.str());
    while (!(stepStream >> parsedInputs.StepAmount)
        || parsedInputs.StepAmount <= 0
        || parsedInputs.StepAmount > parsedInputs.UpperLimit - parsedInputs.LowerLimit)
    {
        cout << "Invalid Step Amount!\n";
        stepStream = GetSingleInput(stepMessage.str());
    }

    return parsedInputs;
}

double CelsiusToFarenheit(const double celsius)
{
    return celsius * 33.8;
}

int main()
{
    auto inputs = GetUserInputs();

    cout << "Celsius" << "\t\t\t" << "Farenheit" << "\n";
    cout << "----------" << "\t\t" << "----------" << "\n";
    cout.precision(9);
    while (inputs.LowerLimit <= inputs.UpperLimit)
    {
        cout << fixed << inputs.LowerLimit << "\t\t" << CelsiusToFarenheit(inputs.LowerLimit) << "\n";
        inputs.LowerLimit += inputs.StepAmount;
    }

    return 0;
}

I'm a C# developer who has been self learning C++. As a result I am likely making simple mistakes on structural design which don't feel odd in C#.

Any commentary would be appreciated, especially as to whether or not my method of checking the input is inefficient.

\$\endgroup\$
  • \$\begingroup\$ Personally, seeing double CelsiusToFarenheit(const double celsius) is weird to me. When dealing with function arguments I only use const if it refers to a reference or a pointer. Otherwise it sort of implies that you think changing the copied object passed to the function would change the original object. This is subjective however. \$\endgroup\$ – twohundredping Sep 3 '15 at 18:09
11
\$\begingroup\$

Don't do this:

using namespace std;

In real life it cause more problems than it is worth. So get out of the habit of using it. It will kick your butt one day. Why is “using namespace std;” considered bad practice?

It is unusual to see a stream being returned.

stringstream GetSingleInput(const string message)

But if it works then fine.
Though I would change the return line.

return stringstream(input); // construct and return on single line.
                            // Easier for the compiler to plant
                            // RVO code.

Use of auto:

auto UpperLimitStream = GetSingleInput(UpperLimitMessage.str());

Not sure I like the use of auto here. Types are important to C++. I prefer to use auto where the type does not matter. But here the type sort of does matter (it has to be a stream (or stream like)).

If your application can't fail. Then don't bother with:

return 0;

main() is special and adds this at the end of main. When I see a missing return 0; at the end. It is an indication that the application should not be failing. If I do see a return 0; I start looking around main for the failure cases.

\$\endgroup\$
  • \$\begingroup\$ Using auto when the type would be too verbose seems good practice to me, but it seems subjective (I am used to dynamic typing) \$\endgroup\$ – Caridorc Sep 3 '15 at 16:18
  • \$\begingroup\$ @Caridorc: Yes it very subjective. The community is still getting used to when the best times to use auto is (we have not really reached a consensus or a set of "rules of thumb yet"). I am still on the conservative end of the spectrum. The reason being I want the compiler to find that error at compile time rather than run-time. \$\endgroup\$ – Martin York Sep 3 '15 at 16:26
  • \$\begingroup\$ Good time: for loops (you don't care about the iterators actual type). Lambdas: You don't care about their types (and if you call it incorrectly that's a compile time error anyway). \$\endgroup\$ – Martin York Sep 3 '15 at 16:28
  • 1
    \$\begingroup\$ The use of auto for me is because it's fairly standard practice in C# at my shop to use them where ever possible to save on long generic types and to make refactoring easier (especially for slight type changes in foreach loops). I don't really have a compelling reason to have used them here, other than habit. Thanks for the feedback. \$\endgroup\$ – BlindGarret Sep 3 '15 at 17:29
13
\$\begingroup\$

Your conversion from celcius is not correct, since 0°F is not 0°C. Something like \$F = \frac{9}{5} C + 32\$ should work for the conversion.

\$\endgroup\$
  • 2
    \$\begingroup\$ Pointing out bugs is a valid review! Welcome to CodeReview :) \$\endgroup\$ – Caridorc Sep 3 '15 at 16:15
1
\$\begingroup\$

A huge part of your code is about receiving number user inputs that are in a certain range. I suggest a function:

double numberInput(prompt, errorText, min, max)

You may reuse it 3 times to reduce duplication.

Example of usage:

numberInput("Enter the min", "min must be a number in range 0->5000", 0, 5000)

 Enter the min
 > hshsh
 min must be a number in range 0->5000
 Enter the min
 > 72
 Program continues...
\$\endgroup\$
0
\$\begingroup\$

The problem statement does not say that the Celsius temperatures should be converted to Fahrenheit, so CelsiusToFahrenheit is not part of the solution.

\$\endgroup\$
  • \$\begingroup\$ Actually, while not explicitly stated in the problem text, the actual site that the problem comes from , linked in the post, gives an expected output of the program. This example shows the formatting of the Celsius and Fahrenheit outputs. \$\endgroup\$ – BlindGarret Sep 4 '15 at 10:59
  • \$\begingroup\$ @BlindGarret - I assumed as much; the title mentions conversion, too. Nevertheless, the problem statement doesn't, and the solution to that problem does not involve converting to Fahrenheit. Part of writing good code is identifying ambiguities in the code's specification and getting them addressed. Writing code based on assumptions about the resolution of an ambiguity can lead to disaster. \$\endgroup\$ – Pete Becker Sep 4 '15 at 13:02

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.