Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is another solution for this challenge.

Problem statement:

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.

I want to learn more about C++, so if there is some cool C++ feature I could should have used, please comment (or include it in your answer).

#import <iostream>
#import <cmath>

#define COLUMN_SEPARATOR "\t| "
#define MAX_TEMP 500
#define MIN_TEMP -500

inline bool between(double x, double max, double min) {
    return max >= x && min <= x;
}

void getInput(double &lower, double &upper, double &step) {
    double temp1, temp2;
    std::cout << "Please enter consecutively the upper and lower limits, both between " <<  MIN_TEMP << " and " << MAX_TEMP << "." << std::endl;
    std::cin >> temp1;
    std::cin >> temp2;
    while (!between(temp1, MAX_TEMP, MIN_TEMP) || !between(temp2, MAX_TEMP, MIN_TEMP)) {
        std::cout << "At least one of the temperatures is out of bounds. Please reenter:" << std::endl;
        std::cin >> temp1;
        std::cin >> temp2;
    }
    upper = std::max(temp1, temp2);
    lower = std::min(temp1, temp2);
    std::cout << "Please enter a positive stepsize, smaller than the difference between the limits." << std::endl;
    std::cin >> step;
    while (step < 0 || step > upper - lower) {
        std::cout << "The stepsize is out of bounds. Please reenter:" << std::endl;
        std::cin >> step;
    }
}

double toFahrenheit(double celsius) {
    return celsius*(9/5) + 32;
}

void printTable(double start, double end, double step) {
    std::cout << "Celsius" << COLUMN_SEPARATOR << "Fahrenheit" << std::endl;
    std::cout << "=======" << COLUMN_SEPARATOR << "==========" << std::endl;
    for (double i = start; i < end; i += step) {
        std::cout << i << COLUMN_SEPARATOR << toFahrenheit(i) << std::endl;
    }
}

int main() {
    double start, end, step;
    getInput(start, end, step);
    printTable(start, end, step);
    return 0;
}

Sample run:

192:Challenges 11684$ ./a.out Please enter consecutively the upper and lower limits, both between -500 and 500.
3.692
65.937
Please enter a positive stepsize, smaller than the difference between the upper and lower limit.
5.3729
Celsius   | Fahrenheit
=======   | ==========
3.692 | 35.692
9.0649    | 41.0649
14.4378   | 46.4378
19.8107   | 51.8107
25.1836   | 57.1836
30.5565   | 62.5565
35.9294   | 67.9294
41.3023   | 73.3023
46.6752   | 78.6752
52.0481   | 84.0481
57.421    | 89.421
62.7939   | 94.7939
share|improve this question
    
    
How did this work with #import? It's supposed to be #include. –  Jamal Jul 29 at 16:50
    
@Jamal In short, #import is a non-portable #include. –  Schism Jul 29 at 16:53
    
@Jamal Oops, too much Objective-C. But I swear it compiles with g++ -Wall tempConverter.cpp. Without warnings/errors/compiler complaints. –  11684 Jul 29 at 16:53
    
@Schism: Okay. I've never seen it used before, but an SO post tells me that it's not standard C++. –  Jamal Jul 29 at 16:54

4 Answers 4

up vote 6 down vote accepted

You don't check for invalid input.

std::cin >> temp1;
std::cin >> temp2;

What if I type BLA BLA<enter> on the input?

As user input is line based. Most programers decide to get a single value at a time.

std::cout << "Please enter consecutively the upper and lower limits, both between " <<  MIN_TEMP << " and " << MAX_TEMP << "." << std::endl;

Now your technique is not wrong. But you definitely make it harder for your self to validate the input and user interaction is not that great as they are used to typing one value return (and getting feedback on that value).

I would change that getInput() so that each value is queried for separately (and use a function to get the value).

    lower  = getUserInput("Please Enter the lower limit of the table", [](int x){return x >= MIN_TEMP;});
    upper  = getUserInput("Please Enter the upper limit of the table", [](int x){return x <= MAX_TEMP;});
    step   = getUserInput("Please Enter the step size",                [](int){return true;});

Don't use macros for constants.

#define MAX_TEMP 500
#define MIN_TEMP -500

That's really old school C. Macros have no concept of scope or type. As such they can potentially clash with other people's macros. Prefer to use const values.

static const int maxTemp = 500;
static const int minTemp = -500;

Or if you are using C++11 and above.

static constexpr int maxTemp = 500;
static constexpr int minTemp = -500;

As a physics persons. You may find that -500 is too low a value (you can not cool things to that temperature ( 0 Kelvin is the lowest temperature theoretically (though there have been experiments that show a temperature a few fractions below this but that has more to do with how we measure the temperature and people are still arguing about it))).

share|improve this answer
    
So getUserInput() uses the lambda to check if the input is valid? –  11684 Jul 29 at 17:26
    
@11684: It was just a suggestion. I am sure you can think of any number of other ways to do it. –  Loki Astari Jul 29 at 18:20
    
Of course, but that is way more elegant than what I had, plus it means using a cool C++ feature (lambdas), which is why I did this "challenge". –  11684 Jul 29 at 18:30

Looks like there are no Americans here. 30 degrees Celsius is about 90 degrees Fahrenheit, so something must be wrong in the conversion. Indeed, (9/5) is evaluated (at compile time) as an integer division, yielding 1. Change it to (9.0/5.0).

share|improve this answer
2  
Whether it is done at compile time or run time is an implementation detail. But it is (as you correctly pointed out) integer division; which results in an integer. I should have seen that. Good eyes. –  Loki Astari Jul 29 at 18:22
    
Or simply omit the parentheses and let operator precedence do its thing. But changing the literals to the correct type is probably a better solution. –  Aurelius Jul 29 at 19:25
  • One fundamental thing is that macros should used as constants:

    #define MAX_TEMP 500
    #define MIN_TEMP -500
    

    should use the const keyword:

    const int max_temp = 500;
    const int min_temp = -500;
    

    Since macros are commonly all-uppercase, these can use camelCase or snake_case.

    You could also put min_temp above max_temp for a bit more readability.

  • Keep in mind that the inline keyword is mostly a compiler hint. In other words, the compiler is free to ignore you if it chooses not to take that hint for whatever reason.

    Side-note: I would've expected inline to also be used for toFahrenheit(), since it is also a single-line function. But like I said, the compiler may still leave it out.

  • It may be a little more readable to change this:

    while (step < 0 || step > upper - lower)
    

    to this (with more parenthesis):

    while ((step < 0) || (step > upper - lower))
    

    This could make it easier to tell that both sides are being compared with ||.

  • Try not to declare/initialize variables on the same line:

    double start, end, step;
    

    If you end up adding additional ones, it'll just make the line longer and longer. Instead, have each one on a separate line:

    double start;
    double end;
    double step;
    
  • You don't need to explicitly return 0 at the end of main(). Reaching this point always indicates success termination, so the compiler will do the return here for you.

share|improve this answer
    
Should temp1 and temp2 be on their own lines too? –  11684 Jul 29 at 17:06
    
@11684: They could, though it's not that crucial since they're very similar and there can only be two of them. My comment on that was primarily a general one (when you have more variables). –  Jamal Jul 29 at 17:08

That is to say, it will include the file at most once.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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