Critique What I've Relearned?

[Shanked]

So, now I've gone through and relearned a lot of basic stuff. There's a lot more to learn, but I'd like to gauge my progress so far.

Here's what I've relearned:
Variables, I/O, Conditional Statements, Loops, and Functions

And here is a little program I made that includes all of those things. I'm looking for critiques on everything from redundancy checks to nit picking style and technique. If you think I can learn from your comments, and they aren't phrased in a brutal way, then please post.

#include <iostream>
#include <sstream>
#include <string>

using namespace std;

int counter=1;

int instructions (char res1) 
{
    string res2;
    int inst_loop, inst=1;
    if (res1=='y')
      {
                 while (inst==1)
                 {
                       cout << "The instructions are simple:"
                            << endl << endl
                            << "When the program prompts you, follow its instructions..."
                            << endl 
                            << "Choosing up (u) will make the count increase."
                            << endl
                            << "Choosing down (d) will make the count decrease."
                            << endl 
                            << "When Counter reaches 0, it will terminate."
                            << endl << endl;
                       cout << "Type \"ready\" to continue\n";
                       cin >> res2;
                       if (res2=="ready")
                       {
                                         inst=0;
                                         cout << "Have fun!\n";
                       }
                       else
                       {
                           cout << "Let's try again...\n\n";
                       }
                 }
             inst_loop=0;
        }
        else if (res1=='n')
        {
           cout << "On with Counter then!\n\n";
           inst=0;
           inst_loop=0;
        }
        else
        {
         cout << "Not an acceptable response, try again.\n(y/n)";
         cin >> res1;
         inst_loop=1;
        }

return inst_loop;
}

int counters()
{
    int program=1;
    char res3;
    
     cout << "Counter is: "<< counter << endl
           << "Up or down? (u/d)" << endl;
      cin >> res3;
      if (res3=='u')
      {
           counter++;
      }
      else if (res3=='d')
      {
           counter--;
      }
      else
      {
          cout << "Incorrect response, try again...\n";
      }
      
      if (counter<=0)
      {
                     program=0;
                     cout << "Counter has reached 0, Thank you for playing!\n";
                     system ("PAUSE");
      }
      else
      {
          program=1;
      }

return program;
}

int main ()
{
    int program=1, inst_loop=1;
    char res1;
    string res2;
    
    cout << "Welcome to Counter!" 
         << endl
         << "Would you like to read the instructions?(y/n)\n";
    cin >> res1;
    while (inst_loop==1)
    {
      inst_loop = instructions(res1);
    }
    while (program==1)
    {
     program = counters();
    }
              
    return 0;
}

[bovinedragon]

couple of quick things that I noticed:

- You are using 'int' to hold a 1 or 0 in many places(like program, inst_loop), you should be using 'bool'
-There is no reason to define res2 in the main(), you are not using it there, do you know about scope?

[Kheun]

Here's my 2 cents.

1. Avoid using global variables. It may cause global namespace conflict when 2 global variables share the same name in 2 different files.
2. Please be informed that std::endl is also used for flushing the content in cout into the screen. In instructions(), you may use "\n" for separating the line and then std::endl for the last line.
3. Since you have initialize the local variable for inst and program to 1, it is not necessary to assign 1 to them again.

[laasunde]

I would recommend case-insensitive input handling. For instance 'y' works but 'Y' does not.

[GCDEF]

I'd still like to see you use white space.

Change if (res1=='y') to
res1 == 'y'

[JohnW@Wessex]

And maybe variable names that better indicate their purpose.

res1 res2 inst_loop inst ???

[Shanked]

@bovine - Good point. I had been using it before, but the program failed for other reasons and I changed it in an attempt to solve the problem. It's easy enough to change back. Yeah, I know about Scope. I tried taking out some of the variables that I knew wouldn't be needed in certain places. I guess I missed that one.

@Kheun - I made the one variable global to avoid passing through parameters. So you're suggesting I should have it passed through as a parameter instead?

Please be informed that std::endl is also used for flushing the content in cout into the screen. In instructions(), you may use "\n" for separating the line and then std::endl for the last line

I'm confused by this. Though, I hope to remember how to flush the screen completely. I figured \n and endl were interchangeable so I used them as such. You're suggesting that one or the other is better in another situation?

Laasunde - Yup, I had wanted to do that. I was certain there was a function or command to do it. But I suppose I can just use OR in the code instead.

GCDEF - Of course. I want it to look neat too, I'm just scared of overdoing it. I tried breaking an output line and it suddenly lost its highlight in the program, so I'm assuming the compiler would no longer have seen it as part of the string.

[Shanked]

And maybe variable names that better indicate their purpose.

res1 res2 inst_loop inst ???

res1 = response 1
res2 = response 2

inst_loop = instruction loop
inst = instruction


What would you suggest?

[GCDEF]

GCDEF - Of course. I want it to look neat too, I'm just scared of overdoing it. I tried breaking an output line and it suddenly lost its highlight in the program, so I'm assuming the compiler would no longer have seen it as part of the string.

I'm just talking about leaving space between variable names and operators. Instead of variable+=1 just write variable += 1, simple stuff like that. When one distinct entity ends and other begins, put a space in there. Readability is very important, which is why I suggested getting the braces and indentation in order too.

[Shanked]

I'm just talking about leaving space between variable names and operators. Instead of variable+=1 just write variable += 1, simple stuff like that. When one distinct entity ends and other begins, put a space in there. Readability is very important, which is why I suggested getting the braces and indentation in order too.

Right, will do. Maybe I should shut the auto off in the program I'm using, because it really screws up a lot. Especially if I try to add more content in later.

[laasunde]

Laasunde - Yup, I had wanted to do that. I was certain there was a function or command to do it. But I suppose I can just use OR in the code instead.

Take a look at the code example provided by exterminator in this thread :
http://www.codeguru.com/forum/showth...se+insensitive

[Shanked]

Not sure what direction he's going. He mentions toupper and tolower, which were the functions I forgot. But then seemed to be talking about problems with them. So should I use them or not?

[Lindley]

I'm confused by this. Though, I hope to remember how to flush the screen completely. I figured \n and endl were interchangeable so I used them as such. You're suggesting that one or the other is better in another situation?

Normally, output is buffered. That means it doesn't always get written immediately. If the output is being written to a terminal it *will* be written immediately in most cases, but if, say, you were redirecting output to a file, then it might not.

The endl both writes a newline and flushes the buffer. For efficiency, you only need to flush the buffer when you don't expect to be printing more text for a while; just a newline on its own is sufficient the rest of the time.

Oh, and cerr (and opposed to cout) is typically unbuffered, so the same rules don't apply there.

[JohnW@Wessex]

res1 = response 1
res2 = response 2

inst_loop = instruction loop
inst = instruction

What would you suggest?

The fact you had to explain them shows that they were not descriptive enough.

response_1
response_2
instruction_loop
instruction

would be better, but for instance...

What does instruction_loop do? It's name doesn't indicate it clearly.
This means that anyone reading the source code must follow the algorithm through to find out what its purpose is.

If it means 'if not equal to zero then repeat the instructions' then make it a 'bool' and call it something like 'repeat_instructions'.
Try to make the code read a little like a sentence.

bool instructions (char response_1) 
{
    string response_2;
    bool repeat_instructions = true;
    bool finished = false;
    
    if (response_1 == 'y')
    {
        while (!finished)
        {
        ...

            if (response2 == "ready")
            {
                finished = true;
                cout << "Have fun!\n";
            }
        }
    
    repeat_instructions = false;

[exterminator]

Not sure what direction he's going. He mentions toupper and tolower, which were the functions I forgot. But then seemed to be talking about problems with them. So should I use them or not?

You did not look into the code sample. Probably, you haven't reached that far with your study but there is a comparator named CaseInsensitiveCompare given that you can use straightaway. Look at how it is being use in main() function. As for the locale, you can just pass in the standard "C" locale object, created as:

std::locale loc("C");
//OR
std::locale loc;

For now, you could even just drop the locale related stuff from that sample, you only need to worry when you need to handle different ones. And when you would need to handle different ones, you will know how to write it.