Thank you for the help, i really appreciate it.
--- In [email protected], "Shyan Lam" <[EMAIL PROTECTED]> wrote: > 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/
