[SOLVED] I would appreciate constructive criticism on this c++ program.

Dogs · 02-26-2010, 01:59 AM

So after getting the OK on "Doing whatever, so long as the screenshot adds up to what the professor specified, and the code compiles", I decided to dig through some libraries and figure out how better I could meet the requirements.

Almost everything that is required is handled already, however the program needs to not die in the event of wrong input. The comments should explain the situation pretty well.

Bolded comments are particular areas of interest.

Code:

#include <iostream>
#include <cctype>

// Definitions for additional modifiers for the Grade

#define BONUS 5
#define FAILURE -10

using namespace std;


// Global variables

int Grade;
char Verified, Eligible;
string FinalGrade;


/* Function prototypes to introduce the compiler to the functions
   that will be used later on (below main) */
   
int Printro();
int GetLetterGrade();
int Printend();

/* I don't know why GetTheGrades() has to be void at this point
   but the compiler would give an error if it were int instead.*/
   
void GetTheGrades();



/* The main function will, in this order, Print the introduction, 
   prompt the user for numerical input for Grade, Perform calculations
   on Grade based on other input entered by the user, determine the 
   letter grade, print the results, and finally it will quit. */

int main()
{
   Printro();
   GetTheGrades();
   GetLetterGrade();
   Printend();
   return 0;
}


// A function to print the introduction

int Printro()
{
  cout << "\t\tNumber to Letter Grade V 3.1415927 - WIP" << "\n\n";
  return 0;
}

// A function to print the final results 

int Printend()
{

  // Is this the correct way to use extern?

  extern int Grade;
  extern char Eligible;
  extern string FinalGrade;
  Printro();
  cout << "Grade is: " << Grade << "\n";
  cout << "Eligible for extra credit: " << Eligible << "\n";
  cout << "Final grade is: " << FinalGrade << "\n";
  return 0;
}


// If-Else-If construct to convert Grade to FinalGrade

int GetLetterGrade()
{
  extern int Grade;
  extern string FinalGrade;
  if(Grade > 100){
    FinalGrade = "A+";}
  else if(Grade >= 90 && Grade <= 100){
    FinalGrade = "A";}
  else if(Grade >= 80 && Grade < 90){
    FinalGrade = "B";}
  else if(Grade >= 70 && Grade < 80){
    FinalGrade = "C";}
  else if(Grade >= 60 && Grade < 70){
    FinalGrade = "D";}
  else if(Grade < 60){
    FinalGrade = "F";}
  return 0;
}
      
      
      
// A function to collect Grade, and determine whether or not user is Eligible for extra credit 
   
void GetTheGrades()
{
  extern int Grade;
  extern char Verified, Eligible;
  cout << "Enter your grade.";
  if(cin >> Grade)
  {
    if(Grade > 100)
    {
      cout << "Is the grade you entered (" << Grade << ") correct? (Enter yY/nN) \n";
      cin >> Verified; 
      switch(toupper(Verified))
      {
          case ('Y'):
            break;
            
            // I need for the result of nN to allow the user to re-enter the grade
            
          case ('N'):
            cout << "WIP\n";
            exit(1);
            
            // I need for invalid entries to allow for the user to re-enter the grade
            
          default:
            cout << "Invalid Entry - WIP\n";
            exit(1);
      }
    }
    
    cout << "\nEligible for Extra Credit? (Enter yY/nN)\n";
    cin >> Eligible;
    switch(toupper(Eligible))
    {
        case ('Y'):
          Grade += BONUS;
          Eligible = 'Y';
          break;
        case  ('N'):
          Eligible = 'N';
          break;
        default:
          Grade += FAILURE;
    }
  
  }
    else
    {
      
      // Same thing as above, needs to restart from the beginning of GetTheGrade()
        // This is the else to if(cin >> Grade)
      
      cout << "You fail. - WIP\n";
      exit(1);
    }

}

fpsasm · 02-26-2010, 02:12 AM

Quote:

Originally Posted by Dogs

// If-Else-If construct to convert Grade to FinalGrade

int GetLetterGrade()
{
extern int Grade;
extern string FinalGrade;
if(Grade > 100){
FinalGrade = "A+";}
else if(Grade >= 90 && Grade <= 100){
FinalGrade = "A";}
else if(Grade >= 80 && Grade < 90){
FinalGrade = "B";}
else if(Grade >= 70 && Grade < 80){
FinalGrade = "C";}
else if(Grade >= 60 && Grade < 70){
FinalGrade = "D";}
else if(Grade < 60){
FinalGrade = "F";}
return 0;
}

I don't do C++, but are you sure you shouldn't be passing by refrence, not by value (where you do 'FinalGrade=...')?

goplexian · 02-26-2010, 03:15 AM

If you want to use exit(1) you need to add:

#include <stdio.h>
#include <stdlib.h>

Also, change GetTheGrades(); to return int instead of void

Then your program will work, or at least it wouldn't work on mine until I made those changes.

Dogs · 02-26-2010, 04:01 AM

When I change GetTheGrades() to int, I get this warning -

Quote:

151: warning: control reaches end of non-void function

graemef · 02-26-2010, 04:22 AM

If you set up a return type then you need to return something.
Thus for you prototype to be

Code:

int GetTheGrades();

Your call would be

Code:

Grade =  GetTheGrades();

and within the function you will need to return a value.

Code:

return Grade;

graemef · 02-26-2010, 04:30 AM

Unless you are explicitly learning about extern I would not encourage its use. The typical approach would be to declare the variables within the main function and then pass them into or return from the other functions.

grail · 02-26-2010, 08:10 AM

Hi Dogs

I hadn't played with C++ forever so this was a bit of fun.

Few pointers (and reiteration of thoughts above):

1. Unless ABSOLUTELY necessary, global variables should be avoided like the plague (pass by reference to update locally set variables)
2. As with graemef, unless you have to use extern i probably wouldn't in this case.
3. You call Printro() from inside Printend(), but it would appear to have no value here (when i ran it the formatting didn't appeal to me so maybe just personal)
4. Without the use of some loops you will not be able to request the user to re-enter any information
5. Finally, your switch for eligible says that if you enter an invalid response (ie YyNn) that you lose 10 marks. This would seem to be illogical

No I am sure I have made heaps of boo boos too, but see what ya think

Code:

#include <iostream>
#include <sstream>
#include <cctype>

// Definitions for additional modifiers for the Grade

#define BONUS 5
#define FAILURE -10

using namespace std;


/* Function prototypes to introduce the compiler to the functions
   that will be used later on (below main) */
   
void Printro();
void Printend(int pi_grade, char pc_eligible, string ps_fgrade);
void GetLetterGrade(int pi_grade, string &ps_fgrade);
void GetTheGrades(int &pi_grade, char &pc_eligible);



/* The main function will, in this order, Print the introduction, 
   prompt the user for numerical input for Grade, Perform calculations
   on Grade based on other input entered by the user, determine the 
   letter grade, print the results, and finally it will quit. */

int main()
{
	int li_grade;
	char lc_eligible;
	string ls_fgrade;

	Printro();
	GetTheGrades(li_grade, lc_eligible);
	GetLetterGrade(li_grade, ls_fgrade);
	Printend(li_grade, lc_eligible, ls_fgrade);

	return 0;
}


// A function to print the introduction

void Printro()
{
  cout << "\t\tNumber to Letter Grade V 3.1415927 - WIP" << "\n\n";
}

// A function to print the final results 

void Printend(int pi_grade, char pc_eligible, string ps_fgrade)
{

  cout << "Grade is: " << pi_grade << "\n";
  cout << "Eligible for extra credit: " << pc_eligible << "\n";
  cout << "Final grade is: " << ps_fgrade << "\n";
}


// If-Else-If construct to convert Grade to FinalGrade

void GetLetterGrade(int pi_grade, string &ps_fgrade)
{
	switch(pi_grade)
	{
		case 100:
		{
			ps_fgrade = "A+";
			break;
		}
		case 99:
		case 90:
		{
			ps_fgrade = "A";
			break;
		}
		case 89:
		case 80:
		{
			ps_fgrade = "B";
			break;
		}
		case 79:
		case 70:
		{
			ps_fgrade = "C";
			break;
		}
		case 69:
		case 60:
		{
			ps_fgrade = "D";
			break;
		}
		default:
		{
			ps_fgrade = "F";
		}
	}
}
      
      
      
// A function to collect Grade, and determine whether or not user is Eligible for extra credit 
 
void GetTheGrades(int &pi_grade, char &pc_eligible)
{
	char lc_verified;
	string ls_tempvalue;
	bool lb_restart;

	do
	{
		lb_restart = false;
		cout << "Enter your grade: ";
		cin >> ls_tempvalue;

		if ( istringstream(ls_tempvalue) >> pi_grade )
		{
			if ( pi_grade < 0 or pi_grade > 100 )
			{
				cout << "Grades must be between 0 and 100 to be valid.\n";
				cout << "Please re-enter value!\n";
				lb_restart = true;
			}
			else
			{
				do
				{
					lb_restart = false;
    					cout << "\nEligible for Extra Credit? (Enter yY/nN) ";
					cin >> lc_verified;

					switch(toupper(lc_verified))
					{
						case 'Y':
						case 'y':
						{
							pi_grade += (pi_grade < 96)?BONUS:(100 - pi_grade);
							pc_eligible = 'Y';
							break;
						}
						case 'N':
						case 'n':
						{
							pi_grade += FAILURE;
							pc_eligible = 'N';
							break;
						}
						default:
						{
							cout << "Incorrect response!!\n Please retry.\n";
							lb_restart = true;
						}
					}
				} while (lb_restart);
			}
		}
		else
		{
			cout << "You have not entered a numerical value!\nPlease retry.\n";
			lb_restart = true;
		}
	} while (lb_restart);
}

johnsfine · 02-26-2010, 08:26 AM

Quote:

Originally Posted by Dogs

Code:

#define BONUS 5
#define FAILURE -10

Using #define that way is ugly in small projects and grows to much worse in large projects. You should learn the alternatives.

The best is replacement usually an enum. A const int is also often OK.

Code:

enum {
BONUS = 5,
FAILURE = -10};

Quote:

Originally Posted by Dogs

Code:

// Global variables

int Grade;
char Verified, Eligible;
string FinalGrade;

As others pointed out, global variables are also bad style. As you move to bigger projects the problems with unnecessary use of globals will grow. It's better to learn now to keep variables within the right scope.

I happen to think #define is even worse than globals (and for closely related reasons). But both are bad in your code.

To repeat an input request after a user error, the simplest construct is a goto. But many people consider goto to be one of the worst sins in programming, so you need to know practical alternatives.

I totally disagree with grail's suggestion for that. Using a bool variable to represent control flow information (to avoid a goto) is common practice, but it is worse than using a goto. It makes the code harder to understand, maintain or debug.

In simple loops, simply using break at the point success is detected is more understandable. That makes the "wrong" user behavior the normal path through the loop (the path that reaches the end of the loop and continues). But that is less confusing than representing control flow in a variable.

In more complicated cases, it is cleaner to put the entire loop into a function, so the success case can be represented with return instead of break. That also allows success to be detected in a switch rather than an if and even for success to be detected inside a nested loop breaking out of two levels.

An alternative (when break or return from the middle of a loop is confusing) is to put just the body of the loop in a function (ask for input, and either return true if it is OK or display an error message then return false if input is bad). Then the caller can have a simple loop until the function returns true.

Dogs · 02-26-2010, 12:24 PM

Awesome. I'm working on it now, and I just went through Grail's revision. A few things in there that I'm really wondering if they're gunna work or not (for example, "or" instead of ||)

and the switch with cases declared like so -

case 99:
case 90:
{
stuff
}

The reason for scratching 10 points if the person enters the wrong information for "Are you eligible for extra credit?" is because it's a requirement for the assignment.

Also, thanks to this thread I've finally grasped what's going on with passing arguments to functions.

Initial Questions:
I haven't been able to make the grade entry fail, however when asked about eligibility, if I enter this sdfkjalsidf^[[18~07y and submit it, the program gives this output

Quote:

Eligible for Extra Credit? (Enter yY/nN) Incorrect response!!
Please retry.

Eligible for Extra Credit? (Enter yY/nN) Grade is: 6
Eligible for extra credit: Y
Final grade is: F

It only crashes when a mixture of int, char, and operator are used. If you submit it without the "y" at the end, it does not crash.

johnsfine · 02-26-2010, 01:07 PM

Quote:

Originally Posted by Dogs

I just went through Grail's revision. A few things in there that I'm really wondering if they're gunna work or not (for example, "or" instead of ||)

and the switch with cases declared like so -

case 99:
case 90:

I'm pretty sure those won't work.

Dogs · 02-26-2010, 01:58 PM

Ok, here's what I've done to Grail's suggestion. Everything works well enough, but

Code:

else if( pi_grade > 100 && lb_restart == false)
        {
          cout << "\nIs the value you entered (" << pi_grade << ") correct? (Enter yY/nN)\n";
          cin >> lc_verified;
          switch(lc_verified)
          {
            case 'Y':
            case 'y':
            {
              break;
            }
            case 'N':
            case 'n':
            {
              lb_restart = true;
              break;
            }
            default:
            {
              cout << "\nIncorrect response!!\n Please retry.\n";
              lb_restart = true;
              break;            
            }
          }

When you enter -1, it jumps to default, does everything in default, and continues instead of restarting.

Code:

#include <iostream>
#include <sstream>
#include <cctype>

// Definitions for additional modifiers for the Grade

#define BONUS 5
#define FAILURE -10

using namespace std;


/* Function prototypes to introduce the compiler to the functions
   that will be used later on (below main) */
   
void Printro();
void Printend(int pi_grade, char pc_eligible, string ps_fgrade);
void GetLetterGrade(int pi_grade, string &ps_fgrade);
void GetTheGrades(int &pi_grade, char &pc_eligible);



/* The main function will, in this order, Print the introduction, 
   prompt the user for numerical input for Grade, Perform calculations
   on Grade based on other input entered by the user, determine the 
   letter grade, print the results, and finally it will quit. */

int main()
{
  int li_grade;
  char lc_eligible;
  string ls_fgrade;

  Printro();
  GetTheGrades(li_grade, lc_eligible);
  GetLetterGrade(li_grade, ls_fgrade);
  Printend(li_grade, lc_eligible, ls_fgrade);

  return 0;
}


// A function to print the introduction

void Printro()
{
  cout << "\t\tNumber to Letter Grade V 3.1415927 - WIP" << "\n\n";
}

// A function to print the final results 

void Printend(int pi_grade, char pc_eligible, string ps_fgrade)
{

  cout << "Grade is: " << pi_grade << "\n";
  cout << "Eligible for extra credit: " << pc_eligible << "\n";
  cout << "Final grade is: " << ps_fgrade << "\n";
}


// If-Else-If construct to convert Grade to FinalGrade

void GetLetterGrade(int pi_grade, string &ps_fgrade)
{
    if(pi_grade > 100){
     ps_fgrade = "A+";}
    else if(pi_grade >= 90 && pi_grade <= 100){
     ps_fgrade = "A";}
    else if(pi_grade >= 80 && pi_grade < 90){
     ps_fgrade = "B";}
    else if(pi_grade >= 70 && pi_grade < 80){
      ps_fgrade = "C";}
    else if(pi_grade >= 60 && pi_grade < 70){
      ps_fgrade = "D";}
    else if(pi_grade < 60){
      ps_fgrade = "F";}
  }

      
      
      
// A function to collect Grade, and determine whether or not user is Eligible for extra credit 
 
void GetTheGrades(int &pi_grade, char &pc_eligible)
{
  char lc_verified;
  string ls_tempvalue;
  bool lb_restart;

  do
  {
    lb_restart = false;
    cout << "\nEnter your grade: ";
    cin >> ls_tempvalue;

    if ( (istringstream(ls_tempvalue) >> pi_grade) && (lb_restart == false) )
    {
        if ( pi_grade < 0 )
        {
            cout << "\nGrades must not be less than zero.\n";
            cout << "Please re-enter value!\n";
            lb_restart = true;
        }
        else if( pi_grade > 100 && lb_restart == false)
        {
          cout << "\nIs the value you entered (" << pi_grade << ") correct? (Enter yY/nN)\n";
          cin >> lc_verified;
          switch(lc_verified)
          {
            case 'Y':
            case 'y':
            {
              break;
            }
            case 'N':
            case 'n':
            {
              lb_restart = true;
              break;
            }
            default:
            {
              cout << "\nIncorrect response!!\n Please retry.\n";
              lb_restart = true;
              break;            
            }
          }
        }
              
        if(lb_restart == false)
        {
          cout << "\nEligible for Extra Credit? (Enter yY/nN)\n ";
          cin >> lc_verified;
      
          switch(lc_verified)
          {
            case 'Y':
            case 'y':
            {
              pi_grade += BONUS;
              pc_eligible = 'Y';
              break;
            }
            case 'N':
            case 'n':
            {
              pc_eligible = 'N';
              break;
            }
            default:
            {
              cout << "\nIncorrect response!\n10 point deduction applied to Final Grade.\n";
              pi_grade += FAILURE;
            }
          }        
        }
   }  
   else
   {
     cout << "You have not entered a numerical value!\nPlease retry.\n";
     lb_restart = true;
   }
 } while (lb_restart);
}

grail · 02-26-2010, 11:09 PM

Well I have gone back and reviewed both of our solutions.

Firstly, thank you to the others for pointing out my own errors (like i said it has been a while)

Anyhoo:

1. The case statements for the numbers was incorrect and if-then-else is the way to go
2. The reason "sdfkjalsidf^[[18~07y" works is each character is processed because our variable is of type char so cin keeps pumping them in till it finds a valid
response of gets to the end. My solution here was to assign the input to a string and test the length (see code below)

Code:

do
				{
					lb_restart = false;
    					cout << "\nEligible for Extra Credit? (Enter yY/nN) ";
					cin >> ls_tempeligible;

					if (ls_tempeligible.length() != 1 )
					{
						cout << "You have entered too many characters!!\n Please retry.\n";
						lb_restart = true;
					}
					else
					{
						lc_verified = ls_tempeligible[0];
						switch(toupper(lc_verified))
						{
							case 'Y':
							{
								pi_grade += (pi_grade < 96)?BONUS:(100 - pi_grade);
								pc_eligible = 'Y';
								break;
							}
							case 'N':
							{
								pi_grade += FAILURE;
								pc_eligible = 'N';
								break;
							}
							default:
							{
								cout << "Incorrect response!!\n Please retry.\n";
								lb_restart = true;
							}
						}
					}
				} while (lb_restart);

Hope this helps.

johnsfine · 02-27-2010, 09:59 AM

Quote:

Originally Posted by grail

Well I have gone back and reviewed both of our solutions.

I don't think you should be providing code for a homework question. It is more appropriate to provide explanations, concepts and problem diagnosis, but let the OP write the code.

Quote:

2. The reason "sdfkjalsidf^[[18~07y" works is each character is processed because our variable is of type char so cin keeps pumping them in till it finds a valid
response of gets to the end. My solution here was to assign the input to a string and test the length (see code below)

I don't use cin >> much (partly because of this kind of issue). So I don't know the best approach to dealing with excess and/or garbage input.

In other threads, I have seen suggestions to flush cin before displaying the next prompt so any excess input will be dumped and won't be taken for the next cin >>. That prevents your program from being tested via an input file, which I think is a flaw.

I think it is most correct to read and process an entire line of input for the response from each prompt. I don't think cin >> to a string will always take an entire line. I think there are other delimiters that might cause it to stop and leave the remainder of input for the next cin >>. So I prefer using some method that reads an entire line into a string, then examine the whole string to see if it is valid input.

tuxdev · 02-27-2010, 10:04 AM

I use std::getline to read a whole line of input. Took me forever to find that function even existed!

Dogs · 02-27-2010, 10:14 AM

Quote:

Originally Posted by johnsfine

I don't think you should be providing code for a homework question. It is more appropriate to provide explanations, concepts and problem diagnosis, but let the OP write the code.

I don't use cin >> much (partly because of this kind of issue). So I don't know the best approach to dealing with excess and/or garbage input.

In other threads, I have seen suggestions to flush cin before displaying the next prompt so any excess input will be dumped and won't be taken for the next cin >>. That prevents your program from being tested via an input file, which I think is a flaw.

I think it is most correct to read and process an entire line of input for the response from each prompt. I don't think cin >> to a string will always take an entire line. I think there are other delimiters that might cause it to stop and leave the remainder of input for the next cin >>. So I prefer using some method that reads an entire line into a string, then examine the whole string to see if it is valid input.

I found his code to be extremely informative. It's not often that I have an experienced programmer write code that fits in place of my existing code, to illustrate how it works. I'm better at learning by reverse engineering than by reading manuals anyhow.

I do have some questions, though.

What was the question you were asking yourself when you came up with

Code:

(istringstream(ls_tempvalue) >> pi_grade)

?

What problems were you facing?