12
\$\begingroup\$

The goal for this project was to:

Write a program that continues to asks the user to enter any number other than 5 until the user enters the number 5. Then tell the user "Hey! you weren't supposed to enter 5!" and exit the program.

★ Modify the program so that after 10 iterations if the user still hasn't entered 5 will tell the user "Wow, you're more patient then I am, you win." and exit.

Requires:

  • variables, data types, and numerical operators
  • basic input/output
  • logic (if statements, switch statements)
  • loops (for, while, do-while)

(I'm not doing the 2 stars one yet)

My questions are:

  1. How can I optimize my code in every possible way? (efficiency, readability, etc)
  2. How can I improve the code syntax?

#include <iostream>  
using std::cout;
using std::cin; 

int main()
{

    int UserNumber = 0;
    int k = 1;
    while (k < 11)
    {
        cout << "\nEnter any number other than 5: ";
        cin >> UserNumber;

        if (UserNumber == 5)
        {
            cout << "\n\n\nHey! You weren't supposed to enter 5!\n\n\n";
            exit(0);
        }

        else if (k == 10)
        {
            cout << "Wow, you're more patient then I am, you win.\n\n\n";
            exit(0);
        }
        k++;
    }

}
\$\endgroup\$
  • 1
    \$\begingroup\$ Replace your first exit with break and the second exist is not necessary. \$\endgroup\$ – Neil Kirk Oct 24 '14 at 19:55
  • \$\begingroup\$ @NeilKirk Good idea! \$\endgroup\$ – Chantola Oct 24 '14 at 20:23
  • 6
    \$\begingroup\$ The question already has the error, but for the record, it is "more patient than I am". \$\endgroup\$ – Ingo Bürk Oct 25 '14 at 18:45
23
\$\begingroup\$

A simple rule I (and other) try to apply is : define things in the smallest possible scope.

For instance, int UserNumber = 0; can be moved inside the loop.

Similarly, k can be moved once you've made your loop a for loop : for(int k = 1; k < 11; k++).

Then, computer people love counting from 0. If you write : for(int k = 0; k < 10; k++), I am used to this and I know straight-away that there will be 10 iterations. If you re-index your loop, you'll need to convert else if (k == 10) into a else if (k == 9).

Then, you'll realise that there is no need for this check, you can just move this out of the loop.

\$\endgroup\$
15
\$\begingroup\$

Given that the objective of the program is to waste time, I'm not sure that time efficiency is the thing to optimize for :P

As far as anything else goes though there's room for improvement. I think that a for loop makes more sense in this particular case.

for(int k = 0; k < 10; k++)
{
    cout << "\nEnter any number other than 5: ";
    cin >> UserNumber;

    if (UserNumber == 5)
    {
        cout << "\n\n\nHey! You weren't supposed to enter 5!\n\n\n";
        exit(0);
    }
}

cout << "Wow, you're more patient then I am, you win.\n\n\n";
exit(0);
\$\endgroup\$
8
\$\begingroup\$
  • UserNumber shouldn't be capitalized, it's not class.
  • Just for readability sake I'll consider defining constants instead of magic numbers

    #define FORBIDDEN_INPUT 5
    #define MAX_ATTEMPTS 10
    
  • k may be better named (maybe attempt)?

  • I don't like empty line in if-else statement, I want to know that they should be read together, and I know that, when they are close to each other vertically.
\$\endgroup\$
  • 23
    \$\begingroup\$ Please, don't define constants via the preprocessor. We have so many nice alternatives in the compiler's actual language, like const and constexpr. \$\endgroup\$ – dyp Oct 24 '14 at 18:40
  • \$\begingroup\$ Capitalizing only classes is just a convention. There is no rule whatsoever against it. Anyway it has nothing to do with optimizing the code. Same goes for your remark about the empty line, but I agree on that one :-) It looks really weird. \$\endgroup\$ – Tonny Oct 25 '14 at 16:10
  • 1
    \$\begingroup\$ Regarding the variable name, the C standard is actually to use snake_case, so the name should be user_number (and the name itself should be improved). \$\endgroup\$ – Ingo Bürk Oct 25 '14 at 18:49
  • 1
    \$\begingroup\$ @Tonny Naming is a matter of convention and there are good reasons to not break those conventions without a very good reason. Naming entities against the usual conventions definitely is bad practice – and hence bad code. \$\endgroup\$ – Ingo Bürk Oct 25 '14 at 18:50
4
\$\begingroup\$

One more thing, which nobody seems to have touched on. This is a somewhat stylistic matter, but it's also style that helps convey intent. Remember that programming is 1% for the compiler and 99% for your fellow programmers, including a later-time yourself.

Normally, if you assign a value on declaration, it's because the initial value of the variable is somehow special or important. Maybe it's a reference counter, or a pointer that needs to point someplace special, or whatever. In C++, if you don't explicitly assign a value on declaration, then the initial value of the variable is indeterminate. This is important if you do something with the value before any further assignment. So, for example, in a class,

int some_value = 0;

is potentially very different from:

int some_value;

if before you make any explicit assignment you perform some operation that reads the value of it, like perhaps:

some_value++;

More generally, in principle

int x = y;

should be equally valid and not look any more odd when written on the longer form of:

int x;
x = y;

In your case, there is nothing special about the initial value of 0. The initial value of the variable is thrown away before you make any comparison involving the variable, or even more generally, read the value of the variable.

When I read your code, it looks like the initial value 0 should be somehow special or important.

You wouldn't write, for some values of x and 0

int x;
x = 0;
cout << "Give me a number: ";
cin >> x;

so based on this line of reasoning, there is equally little reason to write:

int x = 0;
cout << "Give me a number: ";
cin >> x;

which does exactly the same thing (and which is pretty much what you are doing).

Since the initial value isn't in any way important, I would suggest that you just remove the assignment entirely, keeping only the variable declaration:

int UserNumber;

I would also suggest using a different name for that variable. Perhaps UserInput capitalized to your liking? To me, UserNumber sounds more like some sort of ID number for the user, than a number which the user inputs.

\$\endgroup\$
  • 1
    \$\begingroup\$ Your comments about initialisation are incorrect. Statics are always zero-initialised. That is, static int reference_count; is assuredly the same as static int reference_count = 0;. \$\endgroup\$ – Lightness Races in Orbit Oct 25 '14 at 22:01
  • \$\begingroup\$ @LightnessRacesinOrbit Fair enough point (I'll admit I'm not entirely familiar with the intricate details of C++). It doesn't change the point I was making, but I have edited to hopefully be more correct. Do you feel it is better now? \$\endgroup\$ – a CVn Oct 26 '14 at 17:42
  • \$\begingroup\$ Also @dyp you may want to see the edit. \$\endgroup\$ – a CVn Oct 26 '14 at 17:43
  • \$\begingroup\$ @MichaelKjörling: Yeah, now you've got it. I'm not 100% sure I agree with your suggestion to avoid initialisation when the variable will be assigned to before next read, though I can certainly see why you might adopt this convention and wouldn't argue against it. \$\endgroup\$ – Lightness Races in Orbit Oct 26 '14 at 17:46
  • 2
    \$\begingroup\$ Unfortunately I think you just went a little too far in the wrong direction. Pre-initialising a variable you're going to cin into is conventional. It should be redundant if you have proper error checking around the stream extraction operator call, but often we don't and it's easier to spot that something's wrong if your input is stuck at 0 than if it has an indeterminate value. Sometimes. Anyway, the answer is okay as is. \$\endgroup\$ – Lightness Races in Orbit Oct 26 '14 at 18:15
1
\$\begingroup\$

A couple of things other answers haven't touched upon.

  • Your main function is missing its return statement.
  • exit(int status) should be avoided for normal program flow, use return instead.
\$\endgroup\$
  • 6
    \$\begingroup\$ main() doesn't need a return in C++. The compiler will automatically return 0 at the end if necessary. \$\endgroup\$ – Jamal Oct 26 '14 at 8:16
0
\$\begingroup\$

What happens if the user input isn't a number? std::cin >> UserNumber will set the error indicator of that stream; we should test it, and reset the state if necessary.

std::exit is declared by <cstdlib>, so include that header and qualify the function name. More simply, we could just return directly from main(); no need for the the function call.

\$\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.