Problem
I want to learn how to write good code. Starting off with simple programs for now. So any suggestions regarding style, readability or more efficient ways of implementation, etc. would really be appreciated!
#include <algorithm>
#include <iostream>
using namespace std;
int main(int argc, char** argv)
{
bool running = true;
while(running == true)
{
string input;
cout << "[Enter 'exit' to exit the program]n";
cout << "Enter the text you want to reverse: ";
getline(cin, input);
if (input.compare("exit") == 0)
{
running = false;
break;
}
reverse(input.begin(), input.end());
cout << "The reversed version of your text is: " << input << endl << endl;
}
return 0;
}
Solution
Don’t using namespace std;
It is considered bad practice because of possible name collisions, … Although nobody will hurt you, as this is such a small program 🙂
Don’t compare booleans to booleans
Things like running == true
are completely unnecessary, as running
is already a condition in itself. Just use running
.
You don’t need running
The variable running
is unnecessary. You need one for nested loops (breaking out of both), but for single loops, a single break
and an infinite loop is preferred.
Why are you using compare
?
Seeing input.compare("exit") == 0
would send me right to the docs, as I don’t know what the return value of compare
is.
if (input == "exit");
is more clear.
Don’t flush 2 times in a row (or at all)
std::endl
prints a new line, and then flushes stdout
. This results in a performance hit, so it is better to output an actual newline. In your case:
std::cout << something << "nn";
Note that on some platforms (or if you need to), you have to flush the steam to see the output. In this case it is better to be explicit and use std::flush
.
You’re not using the command line parameters
So why name them? You can just omit the names, or define a main
which takes no parameters:
int main(int, char**) {} //1)
int main() {} //2)
I prefer option 2.
Your code uses implementation specific behavior
std::string
is defined in the string
header. Not every compiler includes string
with iostream
or algorithm
(VS doesn’t), so you have to include it explicitly. Don’t rely on automatic includes.
Technically, you don’t need return 0;
Only for main
, if you omit return 0;
, the compiler will add it for you, so technically, you don’t need to specify it.
In addition to previous answers:
- You should check stream state after input (for example user can hit Ctrl+Z and enter or you can reach end of input file).
return 0;
is not necessary in themain
function.- Also you can move this code to the function and then replace
break;
withreturn;
My version:
#include <iostream>
#include <string>
#include <algorithm>
int main(int argc, char** argv)
{
using std::cout;
using std::endl;
while (true)
{
std::string input;
cout << "[Enter 'exit' to exit the program]n";
cout << "Enter the text you want to reverse: ";
if (!getline(std::cin, input) || input.compare("exit") == 0)
break;
std::reverse(input.begin(), input.end());
cout << "The reversed version of your text is: " << input << endl << endl;
}
}
A small nitpick, in the code below:
string input;
cout << "[Enter 'exit' to exit the program]n";
cout << "Enter the text you want to reverse: ";
getline(cin, input);
I would move string input down next to where it is used in getline(…):
cout << "[Enter 'exit' to exit the program]n";
cout << "Enter the text you want to reverse: ";
string input;
getline(cin, input);
Just helps to keep common code together. In a small example like this it is easy to excuse, but say there were 20 or more statements in between string input and getline. A reader would have to remember that input was defined while reading the other, unrelated, statements.
Small, but something to keep in the back of your mind.
Remove the “type exit to exit” thing. What if someone wants to reverse “exit” → “tixe”? Simply use the standard for ending programs, ^C (Control-C). You don’t even need to implement anything. If someone wants to exit they simply Control-C.
Final:
#include <algorithm>
#include <iostream>
using namespace std;
int main(int argc, char** argv)
{
while(true)
{
string input;
cout << "Enter the text you want to reverse: ";
getline(cin, input);
reverse(input.begin(), input.end());
cout << "The reversed version of your text is: " << input << endl << endl;
}
return 0;
}
In addition to what others said, your program is running now because iostream
happens to have an #include <string>
in it but this is not required. It’s good practice to include what you’re using so you know the program will work in any case, in addition to your 2 includes you should add
#include <string>