How can I improve my Celsius to Fahrenheit Converter code?

[rollcast]

#include <iostream>

using namespace std;

int main()
{
    cout<< "Roll Cast's Celsius to Fahrenheit Converter\n Please enter the minimum temperature in celsius\n";
    double min;
    cin>> min;
    jump:
    cout<< "Please enter the maximum temperature in celsius\n";
    double max;
    cin>> max;
    cout<< "Please enter the number of steps for the temperature range, non zero integer\n";
    int steps;
    cin>> steps;

    if (min > max);
    {cout<< "Your values for max. and min. temperature are impossible please re-enter them.\n";
    goto jump;
    }


    cout<<"Press ENTER to continue\n";
    cin.get();
    cin.ignore();
    return 0;
}

I found a problem on another website to make a temperature converter, http://www.cprogramming.com/challenges/celsius_converter_table.html .

This is the code I have written so far, I haven't looked at the correct solution yet, this basically collects the users inputs and makes sure they are possible, ie. min < max. However when it gets to the goto and it re asks for the min and max it doesn't ask for the no. of steps and just goes into a loop by asking for the 2 inputs over and over.

 

[AlephZero]

However when it gets to the goto and it re asks for the min and max it doesn't ask for the no. of steps and just goes into a loop by asking for the 2 inputs over and over.

That should tell you there is something wrong with the code that tests if min and max are valid, and indeed there IS something wrong with those lines of code.

Hint: there's something in your code that shouldn't be there, but I'm not going to tell you what it is, because you will learn more by finding it yourself.

You don't say what compiler you are using, but a good quality C or C++ compiler would probably give you a warning about what you did wrong, since it's "legal", but very rarely "useful".

 

[rollcast]

I'm using code:blocks to compile the code.

I know the ; after if (min > max) shouldn't be there?

 

[rollcast]

Also would using a loop be a better practice than using goto?

 

[rollcast]

#include <iostream>

using namespace std;

int main()
{
    double max, min;
    int steps;
    cout<< "RollCast's Celsius to Fahrenheit Converter \n";
    do
    {
    cout<< "Please enter the max. temp. in celsius.\n";
    cin>> max;
    cout<< "Please enter the min. temp. in celsius\n";
    cin>> min;
    cout<< "Please enter the number of steps for the range\n";
    cin>> steps;
    if (min <= max && steps >= 0)
    break;
    cout<< "The values you have entered are wrong please check them and re-enter\n";
    }
    while ((min > max || steps < 0));
    cout<< "Press ENTER to continue";
    cin.get();
    cin.ignore();
    return 0;
}

I changed the goto for a do while loop and have added conditionals which ensures that min is always less than or equal to max and that the number of steps between min and max is always a number greater than zero.

 

[DaleSwanson]

Gotos are generally frowned upon. Often people will flatly say you should never use them. However, I feel a lot of people just say that because they hear others say it. In either case, they do lead to bad practices, and are rarely necessary. In my opinion you shouldn't use them until you understand the problems they lead to, and can justify the use.

Break is just a goto in disguise. As with goto, there are cases where it is justifiable to use it. I don't think this is such a case though. I recommend you attempt to reorganize your code so break isn't needed.

Also, unless it is just a formatting issue with the forum, loops should be indented. Get in the habit of indenting everything that is wrapped in curly braces.

 

[rollcast]

Thanks Dale, I think the indent was a formatting issue. I change the conditions in the if so I could remove the remove the break from the loop.

#include <iostream>

using namespace std;

int main()
{
    double max, min;
    int steps;
    cout<< "RollCast's Celsius to Fahrenheit Converter \n";
    do
    {
        cout<< "Please enter the max. temp. in celsius.\n";
        cin>> max;
        cout<< "Please enter the min. temp. in celsius\n";
        cin>> min;
        cout<< "Please enter the number of steps for the range\n";
        cin>> steps;
        if (min > max || steps < 0)
        {
            cout<< "The values you have entered are wrong please check them and re-enter\n";
        }

    }
    while ((min > max || steps < 0));
    cout<< "Press ENTER to continue";
    cin.get();
    cin.ignore();
    return 0;
}

 

[rollcast]

I have finished the conversion and step part of the code but I think I have used a number of work around which are probably not very good practice?

#include <iostream>

using namespace std;

int main()
{
    double maxc, minc, maxf, minf, stepc, stepfv, stepcv;
    int steps;
    cout<< "RollCast's Celsius to Fahrenheit Converter \n";
    do
    {
        cout<< "Please enter the max. temp. in celsius.\n";
        cin>> maxc;
        cout<< "Please enter the min. temp. in celsius\n";
        cin>> minc;
        cout<< "Please enter the number of steps for the range\n";
        cin>> steps;
        if (minc > maxc || steps < 0)
        {
            cout<< "The values you have entered are wrong please check them and re-enter\n";
        }

    }
    while ((minc > maxc || steps < 0));

    if (steps == 0)
    {
        maxf = 1.8 * maxc + 32;
        minf = 1.8 * minc + 32;
        cout<< "Max. Temp. in F ="<< maxf <<endl;
        cout<< "Min. Temp. in F ="<< minf << endl;
    }

    else
    {
        int stepdiv = steps += 1;

        for (int i = 0; i < steps; i++)
        {
            if (i == 0)
            {
                maxf = 1.8 * maxc +32;
                cout<< "Max. Temp. in F ="<< maxf <<endl;
            }
            if (i != 0)
            {
                stepc = (maxc - minc)/ stepdiv;
                stepcv = maxc - (stepc * i);
                stepfv = 1.8 * stepcv + 32;
                cout<< "Step in F ="<< stepfv<< endl;
            }
            if (i + 1 == steps)
            {
                minf = 1.8 * minc + 32;
                cout<< "Min. Temp. in F ="<< minf <<endl;
            }
        }
    }

    cout<< "Press ENTER to continue";
    cin.get();
    cin.ignore();
    return 0;
}

 

[Mark44]

Why are you doing this?

int stepdiv = steps += 1;

I'm not saying it's wrong, but the reason for doing this is not obvious to me, so a comment would be helpful to explain your reasoning.

        for (int i = 0; i < steps; i++)
        {
            if (i == 0)
            {
                maxf = 1.8 * maxc +32;
                cout<< "Max. Temp. in F ="<< maxf <<endl;
            }
            if (i != 0)
            {
                stepc = (maxc - minc)/ stepdiv;
                stepcv = maxc - (stepc * i);
                stepfv = 1.8 * stepcv + 32;
                cout<< "Step in F ="<< stepfv<< endl;
            }
            if (i + 1 == steps)
            {
                minf = 1.8 * minc + 32;
                cout<< "Min. Temp. in F ="<< minf <<endl;
            }
        }

The for loop above could be changed as follows, and should behave the same. Note that this for loop executes one time less.

        for (int i = 0; i < steps - 1; i++) // Revision here.
        {
            if (i == 0)
            {
                maxf = 1.8 * maxc +32;
                cout<< "Max. Temp. in F ="<< maxf <<endl;
            }
            else  // This branch executes if i != 0.
            {
                stepc = (maxc - minc)/ stepdiv;
                stepcv = maxc - (stepc * i);
                stepfv = 1.8 * stepcv + 32;
                cout<< "Step in F ="<< stepfv<< endl;
            }
         }
         // Moved this code outside of loop.
         minf = 1.8 * minc + 32;
         cout<< "Min. Temp. in F ="<< minf <<endl;

 

[DaleSwanson]

Looks good overall. A few things I would note:
First, did you really mean to do this:
int stepdiv = steps += 1;

Break that apart into an assignment and increment statement if you really want to do that. It seems you could accomplish the same thing by changing your for loop to be <=

It seems like those three ifs near the bottom could be else if or nested.

No comments. While you don't need to comment every line, there should be some comments. I like to put comments on the opening curly brace lines {, to explain what that block does. This let's me fold the code up but still see what the blocks do.

Your variables aren't very descriptive. Frankly, I'm guilty of the same thing. Remember that it is easy for you to remember what they represent because you invented them. If you ever want to write code that will be read by others (eg professionally) you should get into the habit of writing out long descriptive names. When you write code that has to be maintained the bulk of its life is spent being read, not written. Therefore, it's best to take more time to write the code if it lessens the time to read it later.

If you plan to mostly write one time use code for yourself then maybe you can keep with the short variable habit. I like to add a comment to each variable deceleration line to explain what it is.