Temperature Converter

[guatemala007]

Hello guys, a little while back maybe a few weeks or so I made a Temp. converter program and would just like to hear your feedback on it good or bad.In case your wondering I'm pretty much a beginner as I started coding for the first time ever in September. Personally I think its pretty good but everybody thinks that about their programs. Oh and if you can find any bugs that would be very helpful thank you. Code is Below:

#include  <stdlib>
#include <iostream>
#include <string>
#include  <stdio>
#include  <conio>
#include  <math>
using namespace std;

void converter       (long double&,string,string);
void unit            (string&,string&);
void converter_unit  (string&,string&);

int main()
{string temp_unit,conversion,loop=" ";
 long double temp;

do{clrscr();
cout<<"Hello, Welcome to the Temperature Converter Application."<<endl;
cout<<"What type of temperature do you want to convert\? (Celsius/Fahrenheit/Kelvin)"<<endl;
cin>>temp_unit; unit(temp_unit,loop);
cout<<"What do you want to convert it to\? (Celsius/Fahrenheit/Kelvin)"<<endl;
cin>>conversion; converter_unit(conversion,loop);
cout<<"What is your temperature in degrees "<<temp_unit<<"\?"<<endl;
cin>>temp;
if (temp=temp){temp;} else {loop="2";}
clrscr();
if (loop=="2")
{clrscr();cerr<<"A wrong data type was entered.";getch();
break;}
converter(temp,temp_unit,conversion);
cout<<"Your temperature is "<<temp<<" degrees "<<conversion<<endl;
cout<<"Would you like to convert another temperature\? (Yes/No)"<<endl;
cin>>loop;
}while (loop=="yes"||loop=="Yes"||loop=="Y"||loop=="y");
cout<<endl<<"Thank you and good-bye. Press any key to close...";
getch();
return 0;
}


void unit(string& temp_unit,string& loop)
{
if (temp_unit=="Celsius"||temp_unit=="celsius"||temp_unit=="C"||temp_unit=="c")
{temp_unit="Celsius";}
else if (temp_unit=="Fahrenheit"||temp_unit=="fahrenheit"||temp_unit=="f"||temp_unit=="F")
{temp_unit="Fahrenheit";}
else if (temp_unit=="Kelvin"||temp_unit=="kelvin"||temp_unit=="K"||temp_unit=="k")
{temp_unit="Kelvin";}
else {loop="2";}
}

void converter_unit(string& conversion,string& loop)
{
if (conversion=="Celsius"||conversion=="celsius"||conversion=="C"||conversion=="c")
{conversion="Celsius";}
else if (conversion=="Fahrenheit"||conversion=="fahrenheit"||conversion=="f"||conversion=="F")
{conversion="Fahrenheit";}
else if (conversion=="Kelvin"||conversion=="kelvin"||conversion=="K"||conversion=="k")
{conversion="Kelvin";}
else {loop="2";}
}

void converter(long double& temp,string temp_unit,string converter_unit)
{
if (temp_unit=="Celsius"&&converter_unit=="Fahrenheit")
{temp=(1.8*temp)+32;}
else if (temp_unit=="Celsius"&&converter_unit=="Kelvin")
{temp+=273.15;}
else if (temp_unit=="Fahrenheit"&&converter_unit=="Celsius")
{temp-=32;
 temp*=5;
 temp/=9;}
else if (temp_unit=="Fahrenheit"&&converter_unit=="Kelvin")
{temp-=32;
 temp*=5;
 temp/=9;
 temp+=273.15;}
else if (temp_unit=="Kelvin"&&converter_unit=="Celsius")
{temp-=273.15;}
else if (temp_unit=="Kelvin"&&converter_unit=="Fahrenheit")
{temp-=273.15;
 temp=(1.8*temp)+32;}
else if (temp_unit==converter_unit)
{temp=temp;}
}

[cire]

After I removed the getch and clrscr calls so I could compile it:

Hello, Welcome to the Temperature Converter Application.
What type of temperature do you want to convert? (Celsius/Fahrenheit/Kelvin)
Celsius
What do you want to convert it to? (Celsius/Fahrenheit/Kelvin)
Fahrenheit
What is your temperature in degrees Celsius?
0
A wrong data type was entered.
Thank you and good-bye. Press any key to close...

Hello, bug.

if (temp=temp){temp;} else {loop="2";}

is equivalent to:

if ( temp == 0 ) loop = "2" ;

Since loop is the poorly named variable that determines if you've entered an invalid temperature type and setting it to "2" apparently indicates you've entered an invalid temperature type, your program doesn't handle converting temperatures from 0 in any unit.

The indentation is non-existent. There's quite a bit of code duplication (unit and converter_unit are literally identical.) Variables are named poorly as we've seen with the loop variable. The brace style is pretty horrific.

But other than the one bug , it seems to work alright.

[TheIdeasMan]

I think you would be better to have a ShowMenu function that asks for a single char as input, and make use of the toupper function. This will save the overly complex testing you have - you could just test for 'C', 'F', or 'K'.

You should have a function that does each type of conversion.

With units conversion programs, if you have n units to be converted, you wind up having n^2 - n or n(n-1) conversion functions, which is a problem if you add more units. It is not too bad because you only have 3 units (6 functions), but another approach is to have 2 conversion functions: Always convert from the input units to a base unit, from there to the target unit. This way you only need 2(n-1) conversion functions. In you r case this would be 4 functions.

Other things:

Don't put multiple statements on one line;

Things like this can be done as 1 statement:

{temp-=32;
 temp*=5;
 temp/=9;
 temp+=273.15;}

HTH

[guatemala007]

Well first off thank you both for identifying my multiple errors and bugs.

Addressing the bug that cire pointed out, It was my poor attempt to create an error message as I thought it would work out well but seeing as that bug showed up I guess it didn't. I will definitely work on that. However, I don't understand what you mean by brace style is horrific, right now I'm in a high school level Intro to C++ class and that's how our teacher taught us to write code. I will probably just make a normal function for the unit and converter_unit variables which encases both of them.

And for the tips by TheIdeasMan I will try out the toupper function as it seems much more effective than my method, and I'll see if i can fit the conversion for multiple units as a seperate function, as well as fix the multiple line statements.

To both, I will re-post my edited code once i fix the bugs and include your tips, thank you.

[Raezzor]

What he meant was the brace style was inconsistent. For instance in the function you have the open and close braces on their own lines, which is fine. However, for the loops and other blocks you include the braces on the same lines as the statement. Honestly, if you only have one statement in a loop or similar structure you are almost better off not using braces at all, but simply indenting the statement.

I don't know why, but many coders seem to think that it's better to cram as much code into as few lines as possible and show little consideration to readability. But, the flip side is stuff like TIM pointed out. There are spots that can be condensed and made more efficient. Me, I like to add a blank line to separate multiple line of code that form logical blocks.

Here's your unit() function made a bit more readable (at least, for me. Formatting style for code is a hotly debated topic among programmers, so there's no single "right" way. But there are some ways that most people can read more easily then if you don't follow a pattern at all.)

void unit (string& temp_unit, string& loop) // I like to space parameters by a space after the comma
{
    if (temp_unit=="Celsius"||temp_unit=="celsius"||temp_unit=="C"||temp_unit=="c")
        temp_unit="Celsius"; //Single statements don't require braces inside loops
    else if (temp_unit=="Fahrenheit"||temp_unit=="fahrenheit"||temp_unit=="f"||temp_unit=="F")
        temp_unit="Fahrenheit";
    else if (temp_unit=="Kelvin"||temp_unit=="kelvin"||temp_unit=="K"||temp_unit=="k")
        temp_unit="Kelvin";
    else 
        loop="2";
}

Now, in my programs, I might go a step further and separate each of the loops by a blank line, but it's not really necessary. This should be more then enough to ease the eye strain for most folks. :)

One other thing that you could try is to make separate function for each conversion type. It would cut down on the amount of code inside each function. Then you simply call the proper function using a switch with the temp_unit. This would cut down on all the if/else statements all over. I also agree it might be better to use single char for the user input and a toupper or tolower function. This kind of input would be less prone to user error because, well, let's face it, how many folks can type Fahrenheit of Celsius properly even when it's RIGHT IN FRONT OF THEM ON THE SCREEN? Ok, I admit, I even had to look for fahrenheit. :)

[guatemala007]

@Raezzor, Hahaha yeah I had to use word to spell fahrenheit. I think I will do the single char idea, because of that principle. And I get what your saying about the braces, I guess I will do that because it does make it easier to read. Thank You.

[cire]

guatemala007 wrote:

I don't understand what you mean by brace style is horrific,

Raezzor wrote:

What he meant was the brace style was inconsistent.

Nope. What I meant was the brace style was horrific.

Take this, for instance.

{temp-=32;
 temp*=5;
 temp/=9;}

It's like you're attempting to hide the braces. There are two widely accepted brace styles:

    if (condition) {
        // body
    }

    if ( condition )
    {
         // body
    }

And the one thing that's notable about them is that they're... notable. They're not hidden by/in code. It didn't help matters the auto-formatter in VS didn't handle the style gracefully.