Comments embedded...
> -----Original Message-----
> From: jjdinger42 [mailto:[EMAIL PROTECTED]
> Sent: Tuesday, February 01, 2005 4:01 PM
> To: [email protected]
> Subject: [C-Paradise] Hello
>
>
>
> Hello, i am new to this board and to c++.
> anyway, this question i have is this...
> i wrote this program, and although it is functional, i think it could
> be written simpler. can anyone take a look at it and give me your
> thoughts?
>
> thank you
>
> /* This is a program that generates letter grades
> * and a +, -, or <blank> when given a numerical grade.
> * Although functional I think it could be made easier.
> */
>
> #include <iostream.h>
^^^^^^^^^^
Should be <iostream>
> #include <stdlib.h>
^^^^^^^^
Should be <cstdlib>
> using namespace std;
>
> int num_grade; // number grade
> int grade; // temp place holder
> char final; // letter grade
> char post; // +, -, or <blank>
Try to limit the use of global. There is really no need to use global here.
> int main()
> {
> // intro
> cout << "This one is a little trickier." << endl;
> cout << "You give me the number grade and I will return the
> letter grade." << endl;
>
> // input
> cout << "Please enter the number grade: ";
> cin >> num_grade;
>
> if (num_grade >= 101 || num_grade <= -1)
> cout << "Invalid input" << endl;
Try to use braces even for single line if-statement. It makes your
intention clearer (to you and to others that have to read your code) and
easier to maintain your code.
> else
The 'else' part will only enter when 'num_grade' is between 0 and 100...
> while (num_grade >= 0 && num_grade <= 100) {
...So the above condition checking is redundant. Besides, there is no need
to use 'while' because you're not really running a loop. You always 'break'
at the end of the 'while'-block.
> while (num_grade <= 100 && num_grade >= 91) {
No reason to use 'while' here. A simple 'if' will do.
> final = 'A';
> grade = num_grade - 90;
> if (grade >= 1 && grade <= 3)
> post = '-';
> if (grade >= 4 && grade <= 7)
> post = ' ';
> if (grade >= 8 && grade <= 10)
> post = '+';
> break;
Several things here:
1. Is it really necessary to have multiple 'if' like this? If 'grade' is
already between 1 and 3, the other 'if' will never evaluate to true. A more
correct logic would be:
if (grade >= 1 && grade <= 3)
{
...
}
else if (grade >= 4 && grade <= 7)
{
...
}
else
{
assert(grade >= 8 && grade <= 10)
...
}
The final checking is not necessary if your logic is correct. The purpose
of assert() is to ensure correctness. If the logic is correct, the assert()
would never fail.
2. The indentation for the 'ifs' are wrong. The 'ifs' should at the same
level as the previous statements. Same as the last 'break' statement.
(That's why adding braces should make the code clearer)
3. It seems like the computation of 'post' are identical for different
'num_grade'. You can therefore move and consolidate them at the end (with
some checking for 'F' grade). This also makes it less likely to introduce
typo error when you have to make changes to it.
Alternatively, you can also make the computation of 'post' a function. Then
you don't have to make special check for the 'F' grade.
> }
> while (num_grade <= 90 && num_grade >= 81) {
> final = 'B';
> grade = num_grade - 80;
> if (grade >= 1 && grade <= 3)
> post = '-';
> if (grade >= 4 && grade <= 7)
> post = ' ';
> if (grade >= 8 && grade <= 10)
> post = '+';
> break;
> }
> while (num_grade <= 80 && num_grade >= 71) {
> final = 'C';
> grade = num_grade - 70;
> if (grade >= 1 && grade <= 3)
> post = '-';
> if (grade >= 4 && grade <= 7)
> post = ' ';
> if (grade >= 8 && grade <= 10)
> post = '+';
> break;
> }
>
> while (num_grade <= 70 && num_grade >= 61) {
> final = 'D';
> grade = num_grade - 60;
> if (grade >= 1 && grade <= 3)
> post = '-';
> if (grade >= 4 && grade <= 7)
> post = ' ';
> if (grade >= 8 && grade <= 10)
> post = '+';
> break;
> }
> while (num_grade <= 60 && num_grade >= 0){
> final = 'F';
> post = ' ';
> break;
> }
To illustrate the points above, the code structure is something like this:
if (num_grade <= 100 && num_grade >= 91)
{
final = 'A';
post = ComputePost(num_grade - 90);
}
else if (num_grade <= 90 && num_grade >= 81)
{
final = 'B';
post = ComputePost(num_grade - 80);
}
else if (num_grade <= 80 && num_grade >= 71)
{
...
}
else if (num_grade <= 70 && num_grade >= 61)
{
...
}
else if (num_grade <= 60 && num_grade >= 0)
{
...
}
else
{
/* Error */
}
The last 'else' is the "catch-all" for error.
There are still many magic numbers in the code. This I think is hard to
avoid totally. But you can still make the codes easier to maintain by using
a structure of table to relate the range of 'num_grade' and 'final'.
Something like this:
struct RANGE_LETTER
{
unsigned int Min;
unsigned int Max;
char letter;
};
const RANGE_LETTER RangeLetter[] =
{
{ 100, 91, 'A' },
{ 90, 81, 'B' },
{ 80, 71, 'C' },
{ 70, 61, 'D' },
{ 60, 0, 'F' },
{ }, /* Optional */
};
The last empty brace is optional. This set all the elements to zero and can
be used to check the end of the array.
The table makes changing numeric and letter grade much easier, as the entire
table is within a small section that any error (like overlapping or gaps)
can be easily sported.
I'll leave the implementation as an exercise for those who wish to polish up
their coding skill.
>
> // output
> cout << "Your letter grade is a(n) " << final << post <<
> endl;
> break;
> }
>
> system("PAUSE");
I am not sure if this is required by your instructor but putting a pause or
non-standard getch() actually crippled a well written console program.
A console program should run and end with minimum user intervention. It is
not meant to be run from GUI environment. If you want to see output, open a
command prompt and run the console program from there. Adding a pause at
the end makes the program less useful in a batch environment or run as a
backend process.
HTH
Shyan
>-----------------------------------------~-~>
CHECK THE ARCHIVE BEFORE POSTING!!!! Archive is available at
http://www.eScribe.com/software/C-Paradise/
>------------------------------------------_->
Yahoo! Groups Links
<*> To visit your group on the web, go to:
http://groups.yahoo.com/group/C-Paradise/
<*> To unsubscribe from this group, send an email to:
[EMAIL PROTECTED]
<*> Your use of Yahoo! Groups is subject to:
http://docs.yahoo.com/info/terms/
