Problems looping within function

[KvltKitty]

Hello all,

First time poster here. So I seem to be having some problems grasping how to manage loops within functions. I have a project I am working on for class that wants me to:
1) Have a user enter a sentence
2) Allow the user to choose if they want to
a) Replace a letter with another letter, prompting them to input the first character and then the second they wish to replace
b) Reverse the order of the sentence they entered
c) Change the sentence to all uppercase

This and if they incorrectly select how they want to edit it (given the choice to use H (replace), R (reverse), and U (uppercase) three times then the program prompts them for a new sentence.

This is what I have, and for some reason I can't get the loops to execute properly when I enter the characters that I have set. Also, I can't figure out why if I enter more than one word for s, it seems to print out some of the results multiple times



#include<iostream>
#include<string>
#include<cctype>
using namespace std;


string s;
char editor;
char first;
char second;
int i = 0;


void senEditor() {

while(s != "quit") {
cout << "Please choose an editing operation: " << endl;
cout << "H to replace all characters x with characters y" << endl;
cout << "R to reverse the sentence" << endl;
cout << "U to change the sentence to upper case." << endl;
cin >> editor;

if (editor == 'H') {
cout << "Please enter the first character: " << endl;
cin >> first;
cout << "\nPlease enter the second character: " << endl;
cin >> second;
(i = 0);
while (i <= s.length()) {
if (s[i] == first){
s[i] = second;
(i += 1);
}
else {
(i += 1);
}
cout << s << "\n";
}

}


else if (editor == 'R') {
for (int i = s.length() - 1; i >=0; i--) {
cout << s[i];
}
}

else if (editor == 'U') {
for (int i = 0; s[i] !='\0'; i++) {
s[i] = toupper(s[i]);
}
cout << "\n" << s;
}

while(editor != 'H' || 'h' || 'R' || 'r' || 'U' || 'u' || (i <= 2)) {
cout << "Please use H, R or U to specify the editing choice. Please try again." << endl;
cin >> editor;
(i++);
}
cout << "Please enter next sentence or *quit* to end.\n" << endl;
cin >> s;
}
return;

}






int main(int argc, _TCHAR* argv[]) {

cout << "Welcome to editor. Please enter your first sentence or *quit* to end." << endl;
cin >> s;
senEditor();



return 0;
}





If you guys could just point out where I'm going wrong and how, that would be incredibly helpful. Thanks!

[firedraco]

First, use code tags; it will make your code far easier to read.

while(editor != 'H' || 'h' || 'R' || 'r' || 'U' || 'u' || (i <= 2)) {

This line is incorrect; you can't check for different states like that. You want something like:
while(editor != 'H' || edition != 'h' || /*...*/) {

Also, I can't figure out why if I enter more than one word for s, it seems to print out some of the results multiple times

That's because by default, cin>>some_string will only read the first word. If you input more stuff, it will be held until you use cin again. If you want to get an entire line, you will need to use something like std::getline().

[KvltKitty]

Will do for future reference.

So I fixed what you said, thank you for the help. Still trying to get all this solidified in my mind. The problem I'm running into now is that I run the program through and it works great on the first loop. But at the end I want the program to pause and ask for a new input for the string s, but it simply prints out

Please enter next sentence or *quit* to end.

and jumps straight back to
"Please choose an editing operation: "

without waiting for user input


Here's the updated code:

void senEditor() {

while(s != "quit") {
cout << "Please choose an editing operation: " << endl;
cout << "H to replace all characters x with characters y" << endl;
cout << "R to reverse the sentence" << endl;
cout << "U to change the sentence to upper case." << endl;
cin >> editor;

if (editor == 'H') {
cout << "Please enter the first character: " << endl;
cin >> first;
cout << "\nPlease enter the second character: " << endl;
cin >> second;
(i = 0);
while (i <= s.length()) {
if (s[i] == first){
s[i] = second;
(i += 1);
}
else {
(i += 1);
}

}
cout << s << "\n";
}


else if (editor == 'R') {
for (int i = s.length() - 1; i >=0; i--) {
cout << s[i];
}
}

else if (editor == 'U') {
for (int i = 0; s[i] !='\0'; i++) {
s[i] = toupper(s[i]);
}
cout << "\n" << s;
}

cout << "Please enter next sentence or *quit* to end.\n" << endl;
getline(cin, s);
}
return;

}

[firedraco]

I believe that is because when you use cin to get a single character, you will get the first one, but it will leave the newline in the buffer, so that later when you call getline() it will see that and say "Oh, the user input a blank string." and continue. You'll have to clean out the buffer yourself after you get single characters.

[KvltKitty]

Hmmm well I tried using s.clear(); to empty the buffer, but it still seems to skip over it and go directly to the next operation

[KvltKitty]

Yeah, I've tried a few things and for some reason it's not allowing me to input s a second time

[KvltKitty]

Also, on this line

else {
while (editor != 'H' || editor != 'h' || editor != 'R' || editor != 'r' || editor != 'U' || editor != 'u' || (i <= 2)) {
cout << "\nPlease use H, R or U to specify the editing choice. Please try again." << endl;
cin >> editor;
(i++);

for some reason even if I choose one of the options mentioned, it continues looping, even after i is supposed to have incremented past 2 to terminate the loop.

[KvltKitty]

bump

[firedraco]

cin.clear() does not clear the buffer; you want to use cin.ignore().

[KvltKitty]

Thanks for the help guys
I think I got it working
Is there anything you guys see that's blatantly noobish and could be streamlined better (other than the lack of comment, will use in the future :P )


#include<iostream>
#include<string>
#include<cctype>
using namespace std;


string s;
char editor;
char first;
char second;
int i = 0;
void senEditor(), senReplace(), senReverse(), senUpper();


//Lets user enter sentence and choose operation
void senEditor() {

while(s != "quit") {
cout << "Please choose an editing operation: " << endl;
cout << "H to replace all characters x with characters y" << endl;
cout << "R to reverse the sentence" << endl;
cout << "U to change the sentence to upper case." << endl;
cin >> editor;
int i = 0;

while ((i <= 2) && (editor != 'H') && (editor != 'R') && (editor != 'U')) {
cout << "\nPlease use H, R or U to specify the editing choice. Please try again." << endl;
cin >> editor;
(i++);
}


if (editor == 'H') {
senReplace();
cout << s << "\n";
}

else if (editor == 'R') {
senReverse();
}

else if (editor == 'U') {
senUpper();
}

cin.ignore(10000,'\n');
cout << "\nPlease enter next sentence or *quit* to end.\n" << endl;
getline(cin, s);
}
}
void senReplace() {
cout << "Please enter the first character: " << endl;
cin >> first;
cout << "\nPlease enter the second character: " << endl;
cin >> second;
(i = 0);
while (i <= s.length()) {
if (s[i] == first){
s[i] = second;
(i += 1);
}
else {
(i += 1);
}
}
return;

}


void senReverse() {
for (int i = s.length() - 1; i >=0; i--) {
cout << s[i];
}
return;
}

void senUpper() {
for (int i = 0; s[i] !='\0'; i++) {
s[i] = toupper(s[i]);
}
cout << "\n" << s;
return;
}











int main(int argc, _TCHAR* argv[]) {

cout << "Welcome to editor. Please enter your first sentence or *quit* to end." << endl;
getline(cin, s);
senEditor();
return 0;
}

[firedraco]

1) Use less global variables, try passing arguments to your functions instead
2) void senEditor(), senReplace(), senReverse(), senUpper(); This is a very strange way of declaring functions, I have never seen anyone do that.
3) (i += 1); The extra ( ) looks weird and also why don't you just use ++i?

[TheIdeasMan]

Hi KvltKitty,

First, please always use code tags - they make it easier to read and have line numbers which we can refer to. So Edit your last post, select all the code, then press the <> button on the right under the format menu. If you do it right it should look like the code in my link below. Some other advice just so you know, is to post any compiler messages in full, they show line numbers & tell us exactly what is going on.

http://www.cplusplus.com/articles/jEywvCM9/
http://www.cplusplus.com/articles/z13hAqkS/

I really hate dislike constructs like this:

while ((i <= 2) && (editor != 'H') && (editor != 'R') && (editor != 'U')) {

They are ugly, error prone and not really scalable. What would be much better is a switch statement, as per the code here:

http://www.cplusplus.com/forum/beginner/99203/#msg534078

You can also use switch with int's if your situation needs it.

firedraco is right, your functions only work because they use global variables which is a bad idea. So move your global variables into main. Either send variables as references to functions that need them (using a reference means you can alter the value of that variable ), or what might be easier for you, is to send whatever variables to functions, then return the answer. Make sure the answer returned is assigned to something in main().

I have never understood why people have a return statement in a void function - I know they can be used to return early, but why bother putting one at the end?

Hope all is well at your end