Quote:Original post by ace_lovegrove
Ok, so i know this is pedantic but this is how i would change it if perfection was important.
(Edit: Apparently the reason the board removes source snippets in quotes is that they mess with the quote tags. Grr.)
I know some of this may seem like overkill but that may well be because it is a good habit to get into.
Also im aware mine is more functional than urs is, just got carried away =)
ace
I'm crying here, and not in a good way.
You made a namespace for "file scope globals", introducing the possibility of other people putting extra things into your namespace, when all you really need to do to keep them "file scope" is not mention them in the header.
You made a completely useless constant to represent the zero value. It is not sane to suppose that there will ever be a need to change "DEFAULT_ZERO" in the future, and it adds nothing to readability. Plus, you prove the unintuitiveness of using it consistently, when you write "int totalScore = 0;", forgetting all about your SCORE_INIT_ZERO.
You separate out const char[]'s to represent your text. Okay, I understand the usefulness of moving those into a common location for internationalization purposes - but - optimization shouldn't be a concern at this point; use a real textual type to represent text data - i.e. std::string. Since we're talking internationalization here, it pays to remember that Unicode might become an issue. Outputting a const char[] isn't going to magically convert from UTF-8, you know.
You lost the functionality of labelling the score values "naturally" with english names. Heck, you expect the user not to balk at "Enter score 0".
You accumulate the user information into a useless array (writing to each location once and reading back once). I don't complain on grounds of optimization, but on the grounds that doing that work obfuscates matters - someone reading the code wonders why the array is there.
You missed the opportunity to encapsulate the process of prompting for/getting input, and to ensure that a number is entered.
You missed the opportunity to account for round-off error (integer division discards the remainder).
You kept in the system("PAUSE") call, which doesn't properly belong in console apps, but in a batch file that runs the app (Fruny can sell this better than I).
#include <iostream>#include <limits>using namespace std;// Text used by the program.const string enter = "Enter the ";const string[] const ordinals = {"first", "second", "third"};const string score = " score:";const string error = "That is not a valid score; please enter a number."const string theAverageIs = "The average of the 3 scores is ";// The use of 'sizeof' here is an idiom which determines the length of an// array at compile-time. In the future, when we add more labels to 'ordinals',// we will therefore not have to count them; the compiler does it for us :)const int numScores = sizeof(ordinals) / sizeof(string);int getInput(string prompt) { int result; // At the beginning of each loop, write the prompt, then read input. // The output will always succeed; then we check if the input failed as // the condition for looping. while ((cout << enter << prompt << score) && !(cin >> result)) { // Each time through this loop, we were given a non-numeric input. // So we must write an error message, cout << endl << error << endl; // Fix up cin so that we can read again, cin.clear(); // and skip over the bad input so we don't read it again. cin.ignore(numeric_limits<streamsize>::max(),'\n'); // Now the loop will execute again, showing the prompt and getting new input. } cout << endl; // put space before the next prompt (or answer) // Otherwise, result contains what we need. return result;}int main(int argc, char* argv[]) { int sum = 0; for (int i = 0; i < numScores; i++) { sum += getInput(ordinals); } // In my own code, I would not use these two temporary variables, but instead // just put the expressions right in line in the cout output. This is because // I am comfortable with that sort of thing; I appreciate that beginners // generally find it easier to give names to every quantity in the program. // That's perfectly good practice if your names are good :) int average = sum / numScores; // The modulus ('%') operator returns the remainder in an integer division. int remainder = sum % numScores; cout << theAverageIs << average << " " << remainder << "/" << numScores << endl;}
Good code is at once readable and short, doing everything that it needs to do in only the right places (and avoiding redundancy). My code above only looks long because I have put in a large number of explanatory comments for instructive purposes (I wouldn't personally comment anything except maybe the while loop in getInput()) and because of a couple extra temporary variables (and the separation of the text data out for easier access). Oh, and because of the busy-work you have to do to read numbers in
robustly in C++.
(See this page for reference on this kind of stuff.)
