7
\$\begingroup\$

This is an assignment given in my C++ programming class. I have one main menu with four other menus branching out from it. I would like to make my code as clutter-free as I can, so please review this code and give me feedback.

#include <iostream>
#include <iomanip>
#include <limits>
#include <string>
using namespace std;

struct Selection_Choices
{
string choice1;
string choice2;
string choice3;
};

Selection_Choices lemonade [3];
Selection_Choices shirts [2];
Selection_Choices cookies [3];
int menu_1= 1;
int menu_2= 1;
int menu_3= 1;
double running_Total = 0;

void lemonade_Menu ();
void cookie_Menu ();
void shirt_Menu ();
void purchase_Total();
void purchase_order( double );
void menu_check();

void main ()
{
int menu;
cout <<"\n\n\n\n\n\n\n\n\n\n \t EACH ITEM HAS A PURCHASE LIMIT DUE TO POPULAR DEMAND \n" ;
cout <<"\n\n Please Select From The Following Menu ";
cout <<"\n\n 1. Drink Menu " << endl;
cout <<"\n 2. Cookie Menu  " << endl;
cout <<"\n 3. Shirt Menu " << endl;
cout <<"\n 4. Proceed To Check Out " << endl;
cout <<"\n 5. Exit " << endl;
cout <<"\n Current Selection .......     " ;
cin >>menu;

switch(menu)
{
case 1 :
lemonade_Menu();
break;

case 2 :
cookie_Menu();
break;

case 3 :
shirt_Menu ();
break;

case 4 :
purchase_Total();
break;

case 5 :
exit(0);
break;

default :
cout << "\n\n\n \t\t\t INVALID NUMBER - CHOOSE FROM 1 - 5 " ;
cout << " \n\n ";
cout << " \t\t\t    PRESS ENTER TO CONTINUE ";
cin.ignore(std::numeric_limits<streamsize>::max(),'\n');
cin.get();
main();
}

system ("pause");
}

void lemonade_Menu ()
{
menu_check();
double price = 0 ;
int menu;
cout <<"\n\n\n\n\n\n\n \t LIMIT IS 3 PER PERSON \n" ;
cout <<"\n\n Please Select From The Availible Cup Sizes ";
cout <<"\n\n 1. 8 oz  " << endl;
cout <<"\n 2. 12 oz " << endl;
cout <<"\n 3. 16 oz " << endl;
cout <<"\n 4. Return To Main Menu " << endl;
cout <<"\n Current Selection .......     " ;
cin >>menu;

switch(menu)
{

case 1 :
menu_1++;
price = .50;
for ( int index = 0 ; index < menu_1 ; index++)
{
lemonade[index].choice1 = " Lemonade 8oz  = .50 " ;
}
purchase_order( price );
break;

case 2 :
menu_1++;
price = .75;
for ( int index = 0 ; index < menu_1 ; index++)
{
lemonade[1].choice2 = " Lemonade 12oz = .75 ";
}
purchase_order( price );
break;

case 3 :
menu_1++;
price = 1.00;
for ( int index = 0 ; index < menu_1 ; index++)
{
lemonade[2].choice3 = " Lemonade 16oz = 1.00 ";
}
purchase_order( price );
break;

case 4 :
main();
break;

default :
cout << "\n\n\n \t\t\t INVALID NUMBER - CHOOSE FROM 1 - 4 " ;
cout << " \n\n ";
cout << " \t\t\t    PRESS ENTER TO CONTINUE ";
cin.ignore(std::numeric_limits<streamsize>::max(),'\n');
cin.get();
lemonade_Menu();
}
}

void cookie_Menu ()
{
menu_check();
double price = .75;
int menu;
cout <<"\n\n\n\n\n\n\n \t LIMIT IS 3 PER PERSON \n" ;
cout <<"\n\n Please Select From The Availible Cookies ";
cout <<"\n\n 1. Chocolate Chip " << endl;
cout <<"\n 2. Sugar " << endl;
cout <<"\n 3. Peanut Butter " << endl;
cout <<"\n 4. Return To Main Menu " << endl;
cout <<"\n Current Selection .......     " ;
cin >>menu;

switch(menu)
{

case 1 :
menu_2++;
for ( int index = 0 ; index < menu_2 ; index++)
{
cookies[0].choice1 = " Chocolate Chip Cookie = .75 ";
}
purchase_order( price );
break;

case 2 :
menu_2++;
for ( int index = 0 ; index < menu_2 ; index++)
{
    cookies[1].choice2 = " Sugar Cookie = .75 ";
}
purchase_order( price );
break;

case 3 :
menu_2++;
for ( int index = 0 ; index < menu_2 ; index++)
{
cookies[2].choice3 = " Peanut Butter Cookie = .75 ";
}
purchase_order( price );
break;

case 4 :
main();
break;

default :
cout << "\n\n\n \t\t\t INVALID NUMBER - CHOOSE FROM 1 - 4 " ;
cout << " \n\n ";
cout << " \t\t\t    PRESS ENTER TO CONTINUE ";
cin.ignore(std::numeric_limits<streamsize>::max(),'\n');
cin.get();
cookie_Menu();
}
}

void shirt_Menu ()
{
menu_check();
double price = 0;
int menu;
cout <<"\n\n\n\n\n\n\n \t LIMIT IS 2 PER PERSON \n" ;
cout <<"\n\n Please Select From The Availible Shirts ";
cout <<"\n\n 1. Non-Autographed " << endl;
cout <<"\n 2. Autographed " << endl;
cout <<"\n 3. Return To Main Menu " << endl;
cout <<"\n Current Selection .......     " ;
cin >>menu;

switch(menu)
{

case 1 :
menu_3++;
price = 4.00;
for ( int index = 0 ; index < menu_3 ; index++)
{
shirts[0].choice1 = " Non-Autographed Shirt = 4.00 " ;
}
purchase_order( price );
break;

case 2 :
menu_3++;
price = 8.00;
for ( int index = 0 ; index < menu_3 ; index++)
{
shirts[1].choice2 = " Autographed Shirt = 8.00 ";
}
purchase_order( price );
break;

case 3 :
cout <<" Return To Main Menu " << endl;
main();
break;

default :
cout << "\n\n\n \t\t\t INVALID NUMBER - CHOOSE FROM 1 - 3 " ;
cout << " \n\n ";
cout << " \t\t\t    PRESS ENTER TO CONTINUE ";
cin.ignore(std::numeric_limits<streamsize>::max(),'\n');
cin.get();
shirt_Menu();
}
}



void purchase_order( double current_order)
{

int menu;
cout <<"\n\n What Would You Like To Do ";
cout <<"\n\n 1. Continue Shopping  " << endl;
cout <<"\n 2. Procced To Check Out " << endl;
cout <<"\n Current Selection .......     " ;
cin >>menu;


switch(menu)
{
case 1 :  
if ( running_Total == 0 )
{
    running_Total = current_order;
}
else
{
    running_Total = running_Total + current_order;
}
main();
break;

case 2 :
if ( running_Total == 0 )
{
    running_Total = current_order;
}
else
{
    running_Total = running_Total + current_order;
}
purchase_Total();
break;

default :
  cout << "\n\n\n \t\t\t INVALID NUMBER - CHOOSE FROM 1 - 2 " ;
cout << " \n\n ";
cout << " \t\t\t    PRESS ENTER TO CONTINUE ";
cin.ignore(std::numeric_limits<streamsize>::max(),'\n');
cin.get();
purchase_order;
}
}


void purchase_Total()
{
cout <<"\n\n\n\n Your Order's Summary \n\n ";

for ( int index = 1; index < 3; index ++ )
{
    cout << lemonade[index].choice1 ;
    cout << lemonade[index].choice2 ;
    cout << lemonade[index].choice3 << endl;
}
for ( int index = 0; index < 3; index ++ )
{
    cout << cookies[index].choice1 ;
    cout << cookies[index].choice2 ;
    cout << cookies[index].choice3 << endl;
}
for ( int index = 0; index < 2; index ++ )
{
    cout << shirts[index].choice1 ;
    cout << shirts[index].choice2 ;
    cout << shirts[index].choice3 << endl;
}

cout <<"\n\n\n\n Your Order's Total ...... $ "  <<  running_Total << " \n\n " ;
cout << " \n\n ";
cout << " \t\t\t    PRESS ENTER TO CONTINUE ";
cin.ignore(std::numeric_limits<streamsize>::max(),'\n');
cin.get();
exit(0);
}

void menu_check()
{
if ( menu_1 > 3 || menu_2 > 3 || menu_3 > 2 )
{
    cout << "\n\n\n \t\t YOU HAVE EXCEEDED THE MAXIMUM MENU ENETERIES FOR THIS ITEM " ;
    cout << " \n\n ";
    cout << " \t\t\t    PRESS ENTER TO CONTINUE ";
    cin.ignore(std::numeric_limits<streamsize>::max(),'\n');
    cin.get();
    main();
  }
}
\$\endgroup\$
0
8
\$\begingroup\$

Here's a few things I noted:

  • Don't have using namespace std; in your code. It's almost universally considered a bad practice.

  • The C++ standard states that you should use int main() and not void main(). Bjarne Stroustrup (a creator of C++) stated that this isn't even C++ (or even C!). I'm surprised you could even get this to work, my compiler won't even let me do that.

  • The comments already stated this, but your indentation needs to improve. Right now this program is nearly unreadable.

  • Why do you need so many newline characters in this line? And why are you yelling at me?

    cout <<"\n\n\n\n\n\n\n\n\n\n \t EACH ITEM HAS A PURCHASE LIMIT DUE TO POPULAR DEMAND \n" ;
    

    I personally think that makes the output look ugly and unprofessional.

  • I see a few calls to main() in your code. Use loops in their place instead.

  • Consider using the auto type for some of your variables. For example:

    for ( int index = 0 ; index < menu_3 ; index++)
    

    What if you forget the type of menu_3? Or what if it is some long and complex type name to specify so that index matches it? Here is a better way IMO:

    for (auto index = 0; index < menu_3; ++index)
    

    Note that this is from the C++11 standards.

  • The post-incrementation i++ needs to create a temporary variable to store the original value of i, then performs the incrementation and returns the temporary variable. The pre-incrementation ++i doesn't create a temporary variable. Sure, any decent optimization setting should be able to optimize this away when the object is something simple like an int, but remember that the ++-operators are overloaded in more complicated classes like iterators. Therefore, I would always use the pre-incrementation in a for loop.

\$\endgroup\$
6
\$\begingroup\$

Correctness Issues

main() issues

  • void main() is not valid C++. main() must return int.
  • It is also illegal in C++ to call main() from any point in your program. You will need to reorganize your *_Menu() functions to remove the calls to main().

Bug

At the bottom of purchase_order() (in the default: block) you have a single line:

purchase_order;

My compiler gives me an "unused expression result" warning. Looking at your other functions, it is likely that you meant to call the function:

purchase_order();

If you haven't already, I recommend turning up your warning level (-Wall -Wextra with gcc and clang).

Code Style

using namespace std

There's a lot of example code floating around the internet that includes using namespace std; and it's common among beginners. However, it is a bad idea, particularly in header files (not applicable here, but still good advice) You should prefer to explicitly qualify things from the standard library.

  • Incidentally, you sometimes qualify things with std::, even when you don't need to. Consistency is important.

Indentation

This might be an artifact of pasting here on Code Review, but your code has very poor indentation. Use spaces to indent another level when you enter a new block. This makes it much easier to read your code. Many IDEs even have auto-format capabilities which can automatically perform this indentation.

Naming conventions

In C++, it is a common convention to name types using PascalCase, and variables using either camelCase or snake_case. Your code uses inconsistent casing for your names. It will be easier to read if you pick one convention and stick with it.

Global variables

Your code uses many global variables to keep track of your state. In general, you should avoid this. Instead, you can use better data structures to improve the overall structure of your code. I recommend using fewer void functions (since these must modify global state), and instead have them return the appropriate result as a first step.

\$\endgroup\$
3
\$\begingroup\$

The flow is not quite easy to follow. The program terminates both after the user selects choice 5 and at the end of purchase_Total(). You could instead have choice 4 as another exit choice in main().

Regarding the menu, it could look a little nice with "cancel order" instead of just "exit". It may seem minor, but it's good to imitate aspects of real systems used commonly.

After putting all of that in main(), you can just return EXIT_SUCCESS (same as 0 but more portable) instead of calling exit(0). That call is only needed for terminating the program in another location.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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