Constructive Criticism

[xka]

Hi, I am a total newbie to programming, so I do not know much about it. However I have created this very basic program which converts 2 currencies. I would appreciate it if I received some constructive criticism so I can improve my programming skills.
Thanks,
xka

#include <iostream>
#include <cstdlib>
#include <cmath>
#include <windows.h>    
int yGB();
int gbY();
using namespace std;

int main()
{
    int opChoice;   
    cout << "Welcome to the currency converter \n\n\n";
    Sleep(1000);
starto:  
    cout << "1. British sterling to Japanese Yen. \n";
    cout << "2. Japanese Yen to British sterling. \n";
    
    cin >> opChoice;
          switch(opChoice)
                {
                    case 1:
                         yGB();                    
                         break;
                         
                    case 2:
                         gbY();
                         break;
                         
                    default:
                            cout << " ERROR!ERROR!ERROR!ERROR!ERROR!ERROR!ERROR! \n\n";
                            Sleep(750);
                            goto starto;
                            break;
                }    
          
  return 0;
  system ("pause");  
    }


int yGB()
{
     double gb;
     double yen;
     char sChoice;
     cout << " Please enter the amount you want to convert:  \n\n\n";
     cin >> gb;
     yen = gb * 149.2724;
     cout << " The answer is: " << yen << "\n";   
     Sleep(500);
     cout << " Would you like to go back to the menu? \n\n";
     cin >> sChoice;
     if ( sChoice == 'Y' || sChoice == 'y')
          {
                  main();
           }
     else
         if ( sChoice == 'N' || sChoice == 'n')
            {
                   
            }
     system("pause");   
     }
     
     
int gbY()
{
     double gb;
     double yen;
     char sChoice;
     cout << " Please enter the amount you want to convert: \n\n\n";
     cin >> yen;
     gb = yen/149.2724;
     cout << " The answer is: " << gb << "\n";
     Sleep(500);
     cout << " Would you like to go back to the menu? \n\n";
     cin >> sChoice;
     if ( sChoice == 'Y' || sChoice == 'y')
          {
                  main();
           }
     else
         if ( sChoice == 'N' || sChoice == 'n')
            {
                 getchar();       
            }
     system("pause");   
     }

[jonsca]

Don't use goto. Use a loop instead. This way you can put in an exit option. When getting input like you are use a do/while loop so that you can test for whether you should exit the loop after you've gone through one cycle.

Your function names leave a bit to be desired and the abbreviated names seem to be opposite of what they should be. There's no need to skimp on characters especially for readability. It's the old 6 month rule, could you come back in 6 months and know what exactly they did?
(also why comments are a great idea)

Skip all these sleep calls. By and large the places you have them wouldn't require it anyway. Needing the Windows functions adds a tremendous amount of overhead to your application.

Your functions return an int, but you should actually return the value you got for the conversion to the main program so their return type should be double. When returning to main() use the return (your double variable here); keyword, don't call main again. Skip the "would you like to go back to main" portion of it and just have a return statement (no system("pause") in these functions either).

Your system("pause") in your main program will never see the light of day as you return 0, and you've exited your program. But skip it anyway because (and you had the right idea with the getchar) use cin.get() to have the user press a key to exit. System calls are a disaster because the program is paused, the operating system is brought up (think lots of overhead) the command is executed and the program resumes. cin.get() simply waits for the key to be pressed.

Other than all that, it's a very good first effort. You have a decent concept of functions and breaking down programs and you seem to have input down pat -- so keep at it! Post back with any questions.

P.S. You can adjust this later in your practice but using namespace std; brings dozens of names into the current namespace (meaning you can use cout instead of std::cout) which may cause collisions with the names you have chosen in your code. Just something to be aware of. You can either skip that statement and use std::cout and std::cin or you can use a variant of the using statement using std::cout; using std::cin; thereby qualifying those method names only.

[Nick Evan]

Never use goto. It's bad coding practice. Instead, you can put the entire switch in a while(something) loop. When you want out, just make "something" true.
For example:

bool looping = true;
while (looping) {
   switch (foo) {
      case 1: 
              std::cout << "1!, stop looping!";
              looping = false;
              break;
      default: 
              std::cout << "continue...";
              Sleep(1000);
              break;    
   }
}

[edit]
Crossspost with Jonsca

[Fbody]

Never use goto. It's bad coding practice. Instead, you can put the entire switch in a while(something) loop. When you want out, just make "something" true.
For example:

bool looping = true;
while (looping) {
   switch (foo) {
      case 1: 
              std::cout << "1!, stop looping!";
              looping = false;
              break;
      default: 
              std::cout << "continue...";
              Sleep(1000);
              break;    
   }
}

[edit]
Crossspost with Jonsca

I agree with what you and Jonsca both said, except you might have said this backward. While loops exit on a false condition. Your example syntax appears to be correct though.

[xka]

Thanks for all the help :) But when I tried to change my code it all went horribly wrong :S Can you please point out what I have done wrong :)
Thanks,
xka

#include <iostream>
#include <cstdlib> 
#include <cmath>
    
int YenToGb();
int GbToYen();


int main()
{
    
    int operationChoice;  
    std::cout<< "Welcome to the currency converter \n\n\n";
    std::cout << "1. British sterling to Japanese Yen. \n";
    std::cout << "2. Japanese Yen to British sterling. \n";  
    std::cout << "3. Exit?.                            \n";
    std::cin >> operationChoice;
    bool looping = true;
    while (looping) {
          switch(operationChoice)
                {
                    case 1:
                         YenToGb(); 
                                            
                         break;
                         
                    case 2:
                         GbToYen();
                         break;
                         
                    case 3:
                          looping = false;
                          break;
                    default:
                            std::cout << " ERROR!ERROR!ERROR!ERROR!ERROR!ERROR!ERROR! \n\n";
                           
                            
                            break;
                }    
                }    
  return 0;
  std::cin.get();
    }


int YenToGb()
{
     double gb;
     double yen;
     char sChoice;
     std::cout << " Please enter the amount you want to convert:  \n\n\n";
     std::cin >> gb;
     yen = gb * 149.2724;
     std::cout << " The answer is: " << yen << "\n";   
   
     std::cout << " Would you like to go back to the menu? \n\n";
     std::cin >> sChoice;
     if ( sChoice == 'Y' || sChoice == 'y')
          {
                  
                  return(yen); //I'm unsure what to put here; but it does execute alright when I put that, 
                               // but it says  [Warning] converting to 'int' from double.
                               // Also when I say I do want to return to the menu it starts the function
                               // again.
           }
     else
         if ( sChoice == 'N' || sChoice == 'n')
            {
              
              std::cin.get();    
            }
      
     }
     
     
int GbToYen()
{
     double gb;
     double yen;
     char sChoice;
     std::cout << " Please enter the amount you want to convert: \n\n\n";
     std::cin >> yen;
     gb = yen/149.2724;
     std::cout << " The answer is: " << gb << "\n";
     std::cout << " Would you like to go back to the menu? \n\n";
     std::cin >> sChoice;
     if ( sChoice == 'Y' || sChoice == 'y')
          {
                  return(yen); 
           }
     else
         if ( sChoice == 'N' || sChoice == 'n')
            {
            std::cin.get() ;       // When I say I dont want to return to the menu it goes back to
                                   // the start of the function, and doesnt exit.
            }
       
     }

[Salem]

While we're on the subject of "don't", don't call main recursively as a simple way of generating a loop.

Recursive calls to main() are illegal in C++.

[nezachem]

One more bit of criticism.
YenToGb and GbToYen are practically identical. The only difference is that in one of them a value is multiplied by a factor, while in another it is divided by same factor. When you discovered that something is wrong (for example, a conversion factor changes), you'd have to fix a problem in two places. In programming such situation is called "double maintenance", and should be avoided by all means.
A standard solution is to get rid of one of them, and pass a factor as a parameter:

double convert_currency(factor)
{
    double in, out;
    ....
    std::cin >> in;
    out = in * factor;
    ....
}

and call it as

double factor = 149.2724;
    ....
    convert_currency(factor); // Pounds to yen
    ....
    convert_currency(1.0 / factor); // Yen to pounds

Next, your conversion function does too much. Besides actual conversion, it asks for continuation. It means that the function "knows" in what environment it is called. This is also considered a bad programming practice. Leave the controlling actions for the control loop.

As for your question, you declared your functions as returning int, while they actually return double. So just declare them as double.

[jonsca]

you can use a variant of the using statement using std::cout; using std::cin; thereby qualifying those method names only.

I realize that I might have been confusing. If you put using std::cout; once underneath your #include (though it could be anywhere) you're covered for the remainder of the program in being able to use cout without the std::. Do the same for any members of the std:: namespace that you need. The way you did it is totally fine and is in fact preferred by a lot of people, but if you're just starting out it can be painful.