Code Optimization

[jmorales2012]

Hi everyone. I just started learning C++ last week on my own and have gotten to just before OOP in the tutorial on this site. I decided to start doing my own little projects to reinforce what I've learned so far.

First I made a temperature conversion program and it works just like I want it to, however I want to know if there's a way I could optimize the code any? Is there a better way to write this program given my knowledge base? Any help would be much appreciated. Thanks folks.

#include <iostream>
using namespace std;


float convertCtoF (float celsius) {float faren=(9.0/5.0)*(celsius+32); return faren;}
float convertFtoC (float faren) {float celsius=(5.0/9.0)*(faren-32); return celsius;}

int main()
{
    float temp;
    int choice1=1,choice2=1;

    do {
        cout<<"How would you like to convert temperatures?\n\nPress 1 to convert Celsius to Farenheit\nPress 2 to convert Farenheit to Celsius\nPress 3 to EXIT\n\n";
        cin>>choice1;
        choice2=0;

        while(choice1==1) {
            choice1=0;
            cout<<"\nEnter Celsius temperature: ";
            cin>>temp;
            cout<<"\nTemperature in Farenheit is: "<<convertCtoF(temp)<<"\n\nWould you like to convert another temperature?\n\nPress 1 for YES\nPress 2 for NO\n\n";
            cin>>choice2;
        }
        while(choice1==2) {
            choice1=0;
            cout<<"\nEnter Farenheit temperature: ";
            cin>>temp;
            cout<<"\nTemperature in Celsius is: "<<convertFtoC(temp)<<"\n\nWould you like to convert another temperature?\n\nPress 1 for YES\nPress 2 for NO\n\n";
            cin>>choice2;
        }
    } while(choice2==1);

    cout<<"\n\nThank you. Have a nice day!\n\n";

}

[giblit]

well for one I would use doubles instead of floats they are twice as precise.
Secondly I would try and put the "using" in a local scope and not a global and use just

using cout;
using cin;

instead of using the whole namespace which is a LOT of items that you are not using. Even better which I would suggest is just put the std:: infront of the things in the namespace ex std::cout <<.... std::cin >> ... On another note with lines 5 and 6 just because you can put them on one line doesn't mean you should. Less lines does not mean it is written better/neater. You should use the proper indentation on it like you have in the main function.

double convertCtoF( double celsius )
{
    double faren = ( 9f / 5 ) *( celcius + 32 );/*being lazy you can put .0 to make a float instead
 of f and only one needs to be type float for it to work =p your way is better*/
    return( faren );
}

Also I think for line 18 and 25 you should use if statements and not loops.
You're basically using a loop then breaking it after it has run once.. which a if statement does. Plus you don't have to reset the values to 0 each time when you cin it will change the value it is the same as operator = so using operator = twice makes no sense. You could also use a switch like

switch( choice1 )
{
case 1:
    std::cout << std::endl << "Enter Celsius Temperature: " << std::flush;
    std::cin >> temp;
    std::cout << "Temperature.....";
    break;
case 2:
     ...
     break;
}

You could also even just use one input variable since since they are both using numbers and you are not actually using the number other than to check if statements are true or false. Here is how I would do this program

#include <iostream>
#include <limits>

double celcius_to_fahrenheit( const double &celcius )
{
    return( 9/5.0 * ( celcius + 32 ) );
}

double fahrenheit_to_celcius( const double &fahrenheit )
{
    return( 5/9.0 * ( fahrenheit - 32 ) );
}

void clear_buffer( void )
{
    std::cin.clear();
    std::cin.ignore( std::numeric_limits<std::streamsize>::max() , '\n' );
}

const bool get_input( int &temp )
{
    return( std::cin >> temp );
}

const bool get_input( double &temp )
{
    return( std::cin >> temp );
}

void error_message( void )
{
    std::cerr << "Invalid degree please make another selection." << std::endl;
}

int main()
{
    int input( 0 );
    double temp( 0.0 );
    do
    {
        std::cout << "1 - Celcius to Fahrenheit\n2 - Fahrenheit - Celcius\nAny other key to quit\n> " << std::flush;
        if( !get_input( input ) )
        {
            clear_buffer();
            return( 1 );
        }
        std::cout << input << std::endl << std::endl;
        switch( input )
        {
        case 1:
            while( std::cout << "Please enter your degree in celcius\n> " << std::flush && !get_input( temp ) )
            {
                error_message();
                clear_buffer();
            }
            std::cout << temp << " degrees celcius is " << celcius_to_fahrenheit( temp ) << " degrees in fahrenheit." << std::endl;
            break;
        case 2:
            while( std::cout << "Please enter your degree in fahrenheit\n> " << std::flush && !get_input( temp ) )
            {
                error_message();
                clear_buffer();
            }
            std::cout << temp << " degrees fahrenheit is " << celcius_to_fahrenheit( temp ) << " degrees in celcius." << std::endl;
            break;
        }
        std::cout << "Would you like to run this program again?\nPlease enter 1 to continue...\n> " << std::flush;
        if( !get_input( input ) )
        {
            clear_buffer();
            return( 0 );
        }
        if( input != 1 ) return( 0 );
    } while( true );
}

My way is probably making the simple task way too complicated but hey I was bored...

[TheIdeasMan]

Instead of :

{float celsius=(5.0/9.0)*(faren-32); return celsius;}

You can combine them :

{return (5.0/9.0)*(faren-32); }

I like to use a switch statement for this sort of thing:

http://www.cplusplus.com/forum/beginner/104553/2/#msg564228

To me that is tidier, easy to read & maintain.

Also, it is a good idea to avoid using namespace std; - it brings in the whole std namespace (which is huge) into the global namespace, polluting it & running the risk of function & variable naming conflicts.

Did you know that there are std::left, std::right, std::distance, std::pair to name a few?

So you have some choices as to what to do instead:

1. Put std:: before every std thing - as in std::cout - a lot of people do this exclusively.
2. Have using statements at the top of your file for every std thing you use in that file :

using std::cout;
using std::cin;
using std::endl;
using std::string;
using std::vector;

3. Do a mixture - using statements like above for things used frequently in the file, and something like std::find say, for things not used a frequently.

Hope all goes well.

[andywestken]

0#

I'm in accord with the comments about using; switch statements (or ever if/else if as it's just two conditions to check); and I agree with giblit that you could factor your code to use more functions (though not in their choice.)

(I would also wrap the over long lines (14, 22, and 29). The need for the 80 column rule has gone, not we don't use primitive text editors. But 155 char long lines are way too long! (In this case you can take advantage of the fact that the compiler's preprocessing phase concatentes all adjacent string literals, even across line breaks!

e.g.

char msg[] = "Hello"
             " "
             "world"
             "!";

looks the same to the compiler as

char msg[] = "Hello world!"; )

1#

As giblit has already mentioned, you should use double rather than float for most cases. An exception is when you're using a large vector (or C-style array).

As you might have noticed, all the standard library calls work with double, so you save the compiler from having to cast back and forth between float and double.

And "on x86 processors, at least, float and double will each be converted to a 10-byte real by the FPU for processing."

Float vs Double Performance
http://stackoverflow.com/questions/417568/float-vs-double-performance

2#

But I would keep the local variables in the conversions functions.

Elimination of the local variable is a case of premature optimization as both

float convertCtoF (float celsius) {
    float faren=(9.0/5.0)*(celsius+32);
    return faren;
}

and

float convertCtoF (float celsius) {
    return (9.0/5.0)*(celsius+32);
}

result in identical object code for the optimized build.

The advantage of the form with the local is that if you set a breakpoint on the return, you can see the value of faren in the debugger watch window. (In the one without the local, I just see celsius; with the local I see both celsius and faren.)

Also, having the function's code all on one line also prevents the watch window from working (at least with Visual Studio 2010.)

Andy

PS Both versions of the function cause a compilation warning as it's using double literals rather than float literals. If you want a float literal, rather tham a double literal, then you need to use an f suffix.

// no warnings :-)
float convertCtoF (float celsius) {
    float faren=(9.0f/5.0f)*(celsius+32);
    return faren;
}

[andywestken]

Another take...

A simpler main() function, but more complicated input routines to handle stupid values.

Andy

// ACME (R) Temperature Conversion Program (verion 1.3)
//
// 1. removed question about another go -- the fact it asks again is
//    clear enough and saves the user a few key presses
// 2. added code to prevent entry of non-numeric values for temp
//    causing a problem (try "abc", "12 deg", "12 13 14" with cin >>)
// 3. added a siilar function for choice
// 4. same functions used to handle input errors also allows main loop
//    to be simplified
// 5. used an enum for the condition values
// 6. used a switch statement
// 7. removed using std;
// 8. folded the long lines/the conversion functions (longest line
//    now 89 chars
// 9. switched to doubles
// 10. added assert
// 11. ...
//
// Also:
// - why 1,2,3 and not C, F, and X ?
// - could use templates to "simplify" input validation? (less code/
//   more complicated?)
// - ...

#include <iostream>
#include <sstream> // for std::stringstream
#include <string>  // for std::string, std::getline
#include <limits>  // std::numeric_limits
#include <cassert> // assert

enum Choice {
    Invalid=-1,
    CtoF=1,
    FtoC=2,
    EXIT=3
};

double convertCtoF (double celsius) {
    double faren=(9.0/5.0)*(celsius+32.0);
    return faren;
}

double convertFtoC (double faren) {
    double celsius=(5.0/9.0)*(faren-32.0);
    return celsius;
}

Choice getChoiceFromUser();
double getTempfromUser(const char* prompt);

int main() {
    std::cout<<"ACME (R) Temperature Conversion Program (verion 1.2)\n";

    Choice choice=Invalid;

    do { // while(choice!=EXIT);
        choice=getChoiceFromUser();

        switch(choice) {
            case CtoF: {
                double temp=getTempfromUser("\nEnter Celsius temperature: ");
                std::cout<<"\nTemperature in Farenheit is: "<<convertCtoF(temp)<<"\n\n";
            }
            break;

            case FtoC: {
                double temp=getTempfromUser("\nEnter Farenheit temperature: ");
                std::cout<<"\nTemperature in Celsius is: "<<convertFtoC(temp)<<"\n\n";
            }
            break;

            case EXIT: {
                // do nothing, we're quitting
            }
            break;

            default: {
                assert(false && "assert : should never get here!");
                // as invalid choices should be handled by getChoiceFromUser();
            }
        } // switch(choice)
    } while(choice!=EXIT);

    std::cout<<"\n\nThank you. Have a nice day!\n\n";

    return 0;
}

Choice getChoiceFromUser() {
    for( ; ; ) {
        std::cout<<"\nHow would you like to convert your temperature?\n"
                    "\n"
                    "Press 1 to convert from Celsius to Farenheit\n"
                    "Press 2 to convert from Farenheit to Celsius\n"
                    "Press 3 to EXIT\n\n? ";
        std::string line;
        // read the whole line
        if(std::getline(std::cin, line)) {
            // load it into a istringstream and read it back out using cin
            // so we can find the end of the line (cin never ends!)
            std::istringstream ss(line);
            int choice=0;
            if (ss>>choice) {   // if we converted without error
                if (ss.eof()) { // if we converted the whole string
                    switch(choice) {
                        case CtoF: return CtoF;
                        case FtoC: return FtoC;
                        case EXIT: return EXIT;
                        case Invalid:
                        default: {/* do nothing */}
                    } // switch(choice)
                }
            } // if (ss>>d) ...
        } // if(std::getline...

        // retry!
        std::cout<<"\nInvalid choice. Please try again.\n";
    } // for...
}

// get a valid double value from the user
double getTempfromUser(const char* prompt) {
    for( ; ; ) {
        std::cout<<prompt;
        std::string line;
        // read the whole line
        if(std::getline(std::cin, line)) {
            // load it into a istringstream and read it back out using cin
            // so we can find the end of the line (cin never ends!)
            std::istringstream ss(line);
            double temp=0.0;
            if (ss>>temp) {   // if we converted without error
                if (ss.eof()) { // if we converted the whole string
                    // Success
                    return temp;
                }
            } // if (ss>>d) ...
        } // if(std::getline...

        // retry!
        std::cout<<"\nInvalid temperature. Please try again.\n";
    } // for...
}

// This version can handle proper values (e.g. "12", "13.51") and bad
// string values which begin with a letter (e.g. "fredbill30", "wibble"),
// but can't deal with values which begin with a digit but also contain
// other chars (e.g. "12three", "12 degrees C", "13 13 14".)
//
// This is because it successfully extracts the leading digits but then
// leaves the remaining characters on the line, which can confuse
// subsequent calls to std::cin.
//
// You could just call std::cin.ignore() to ingnore the extra chars, but
// the std::istringstream approach above is (far) better.
// 
//double getTempfromUser(const char* prompt) {
//    for( ; ; ) {
//        std::cout<<"\n"<<prompt;
//        double temp=0.0;
//        if (std::cin>>temp) {
//            // no error, to can return
//            return temp;
//        } else {
//            std::cout<<"\nInvalid temperature. Please try again.\n";
//            std::cin.clear();
//            std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
//        }
//    }
//}  

[jmorales2012]

Thanks everyone for the help! I decided to take off the using namespace std; and have been using std:: instead.

Also I changed out the while loops for switch statements.

I have a question regarding your code Andy.

It has a lot of functions; is this all necessary for a simple program like this? Or is that taking the program to the next step and checking for invalid input?

I really do appreciate the help and all, but my original intent was to just make a simple program that converts temperature. Anything after that will be added in the future as I learn and get comfortable with the language. I will add input-validation and all that good stuff later.

[andywestken]

is this all necessary for a simple program like this?

Probably not. Looking at it now, it does look like I got (a bit) carried away.

But five function is not really a lot (though maybe more than you need.)

And a significant amount of the extra code is in the input routines, so they can handle more or less any erroneous inputs. If you trust your users to enter sensible values, it's not needed.

Andy

PS This is more or less what I converted your code to before I got carried away adding the error handlling. etc.

#include <iostream>

double convertCtoF (double celsius)
{
    double faren=(9.0/5.0)*(celsius+32);
    return faren;
}

double convertFtoC (double faren)
{
    double celsius=(5.0/9.0)*(faren-32);
    return celsius;
}

int main()
{
    int choice=0;

    do {
        std::cout<<"How would you like to convert temperatures?\n"
                   "\n"
                   "Press 1 to convert Celsius to Farenheit\n"
                   "Press 2 to convert Farenheit to Celsius\n"
                   "Press 3 to EXIT\n"
                   "\n";
        std::cin>>choice;

        if(choice==1) {
            std::cout<<"\nEnter Celsius temperature: ";
            double temp = 0.0;
            std::cin>>temp;
            std::cout<<"\nTemperature in Farenheit is: "<<convertCtoF(temp)<<"\n\n";
        }
        else if(choice==2) {
            std::cout<<"\nEnter Farenheit temperature: ";
            double temp = 0.0;
            std::cin>>temp;
            std::cout<<"\nTemperature in Celsius is: "<<convertFtoC(temp)<<"\n\n";
        }
        else if(choice==3) {
            // do nothing, we're exiting
        }
        else {
            std::cout<<"\nInvalid choice.\n\n";
        }
    }
    while(choice!=3);

    std::cout<<"\n\nThank you. Have a nice day!\n\n";

    return 0;
}

[jmorales2012]

Alright awesome. I am thankful you gave me all the advice and help and I will look back to this thread when I challenge myself to add extra features into the program. I'm giving myself incremental challenges so first off I just wanted the basic program, next I want it to be able to handle the bad input, and maybe I'll move into an all-out conversion program (not just temperatures). Just trying to pace myself here and take it step by step.

Thanks again though!



EDIT: Although I do see how the added functions could greatly help in the future if/when I decide to add more to the program. Instead of changing hundreds of different statements, I could just change the functions and be done.