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/showthread.php?t=450815&highlight=string+compare+case+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. :)

[PredicateNormative]

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?


In short,

std::cout << "HelloWorld" << std::endl;

is effectively the equivalent of writing

std::cout << "HelloWorld" << "\n" << std::flush;


Hence, std::endl is a more expensive operation in terms of performance than "\n". Therefore, if you only want to get a new line, then using "\n" is more efficient than using std::endl.

In general, if you want a new line but don't need to flush the buffer, use "\n". If you don't want a new line but need to flush the buffer then use std::flush. If you need a new line and flush the buffer, then use std::endl.

I hope that helps. :)

[Shanked]

Well, here's the new and improved counter:


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

using namespace std;

void instructions (char response_1, bool& repeat_instructions)
{
string response_2;
bool instructions = true;

if (response_1 == 'y')
{
while (instructions == true )
{
cout << "The instructions are simple:\n\n"
<< "When the program prompts you, follow its instructions...\n"
<< "Choosing up (u) will make the count increase.\n"
<< "Choosing down (d) will make the count decrease.\n"
<< "When Counter reaches 0, it will terminate.\n\n";
cout << "Type \"ready\" to continue\n";
cin >> response_2;

if (response_2=="ready")
{
instructions = false;
cout << "Have fun!\n";
}
else
{
cout << "Let's try again...\n\n";
}
}

repeat_instructions = false;
}
else if (response_1 == 'n')
{
cout << "On with Counter then!\n\n";
instructions = false;
repeat_instructions = false;
}
else
{
cout << "Not an acceptable response, try again.\n(y/n)";
cin >> response_1;
repeat_instructions = true;
}
}

void IncrementCounter(bool& program, int& counter)
{
string response_3;
bool quit = false;
char response_3a;

cout << "Counter is: "<< counter << "\n"
<< "Up or down? (u/d)" << "\n";
getline(cin,response_3);

if (response_3 == "u")
{
counter++;
}
else if (response_3 == "d")
{
counter--;
}
else if (response_3 == "quit")
{
program = false;
quit = true;
counter = 0;
}
else
{
cout << "Incorrect response, try again...\n";
}

if (counter <= 0)
{
program = false;
if (quit == true)
{
cout << "Oh well... thanks for playing!\n";
}
else
{
cout << "Counter has reached 0, Thank you for playing!\n";
}
system ("PAUSE");
}
else
{
program = true;
}
}

int main ()
{
bool program = true, repeat_instructions = true;
char response_1;
int counter = 1;
string response_2;

cout << "Welcome to Counter!\n"
<< "Would you like to read the instructions?(y/n)\n";
cin >> response_1;
while (repeat_instructions == true)
{
instructions(response_1, repeat_instructions);
}
while (program == true)
{
IncrementCounter(program, counter);
}

return 0;
}

[GCDEF]

That's not improved. Passing something in as an argument when it can be used as a return value is a step in the wrong direction. You had it right the first time when it was returning a bool.

[Lindley]

The purpose of functions is to encapsulate functionality in a modular way----so you can call them at any point in a program, with some inputs, and get some output(s). That's one reason why global vars are frowned upon; if you use them, you can have functions giving different results for the same input depending on when they're called, which is bad.

There's no harm in designating some parameters as outputs; however, they should be clearly marked as such. Also, in most cases it's best to avoid inout parameters----those which both hold an output *and* may change behavior depending on their initial values.

[Shanked]

That's not improved. Passing something in as an argument when it can be used as a return value is a step in the wrong direction. You had it right the first time when it was returning a bool.

I was under the impression that it didn't matter so much. Easy enough to fix.

[Shanked]

Alright... new and improved!


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

using namespace std;

bool instructions (char response_1, bool repeat_instructions)
{
string response_2;
bool instructions = true;

if (response_1 == 'y')
{
while (instructions == true )
{
cout << "The instructions are simple:\n\n"
<< "When the program prompts you, follow its instructions...\n"
<< "Choosing up (u) will make the count increase.\n"
<< "Choosing down (d) will make the count decrease.\n"
<< "When Counter reaches 0, it will terminate.\n\n";
cout << "Type \"ready\" to continue\n";
cin >> response_2;

if (response_2=="ready")
{
instructions = false;
cout << "Have fun!\n";
}
else
{
cout << "Let's try again...\n\n";
}
}

repeat_instructions = false;
}
else if (response_1 == 'n')
{
cout << "On with Counter then!\n\n";
instructions = false;
repeat_instructions = false;
}
else
{
cout << "Not an acceptable response, try again.\n(y/n)";
cin >> response_1;
repeat_instructions = true;
}
return repeat_instructions;
}

bool IncrementCounter(bool program, int& counter)
{
string response_3;
bool quit = false;
char response_3a;

cout << "Counter is: "<< counter << "\n"
<< "Up or down? (u/d)" << "\n";
cin.sync();
getline(cin,response_3);

if (response_3 == "u")
{
counter++;
}
else if (response_3 == "d")
{
counter--;
}
else if (response_3 == "quit")
{
program = false;
quit = true;
counter = 0;
}
else
{
cout << "Incorrect response, try again...\n";
}

if (counter <= 0)
{
program = false;
if (quit == true)
{
cout << "Oh well... thanks for playing!\n";
}
else
{
cout << "Counter has reached 0, Thank you for playing!\n";
}
system ("PAUSE");
}
else
{
program = true;
}
return program;
}

int main ()
{
bool program = true, repeat_instructions = true;
char response_1;
int counter = 1;
string response_2;

cout << "Welcome to Counter!\n"
<< "Would you like to read the instructions?(y/n)\n";
cin >> response_1;
while (repeat_instructions == true)
{
repeat_instructions = instructions(response_1, repeat_instructions);
}
while (program == true)
{
program = IncrementCounter(program, counter);
}

return 0;
}

[bovinedragon]

-There is no reason to pass 'repeat_instructions' to instructions(), as you are not using that value in that function. The 'repeat_instructions' in the instructions() should be a local variable. Same with 'program' in IncrementCounter()

-As for your main loop, i would probably write something like:

while( instructions(response_1) )
{;}
while( IncrementCounter(program, counter) )
{;}

but people will probably say that this is less readable, but you can see how return values can be used

[GCDEF]

I was under the impression that it didn't matter so much. Easy enough to fix.

No, and I gave you an example why. Functions are often used as part of expressions. For example, if you had a function that did some kind of calculation and gave you a number, you could write it and use it as


int GetSomeNumber()
{
// do some calclations
return number;
}

or

void GetSomeNumber(int& number)
{
// do some calculations and set number's value
}


To use the first function you could simply use it in an expression as

int newNumber = GetSomeNumber() * 2;

The second form would be

int newNumber;
GetSomeNumber(newNumber);
newNumber *= 2;

Three lines of code instead of one, and not the way C and C++ coders are used to thinking.