21
\$\begingroup\$

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;
}
\$\endgroup\$
  • 4
    \$\begingroup\$ Almost never compare with booleans while (running) or while (!found). \$\endgroup\$ – Joop Eggen Sep 23 '16 at 8:39
  • \$\begingroup\$ It doesn't handle unicode (or any multi-byte encoding) \$\endgroup\$ – CodesInChaos Sep 23 '16 at 14:26
  • \$\begingroup\$ @JoopEggen why not? I hope you didn't mean to say never compare two booleans against each other \$\endgroup\$ – Celeritas Sep 25 '16 at 8:47
  • \$\begingroup\$ @Celeritas comparing booleans always is a rare enjoyment, usable for equivalence and more. Therefore that "almost." But in case of (running == false) == false one can someone hear thinking. \$\endgroup\$ – Joop Eggen Sep 25 '16 at 15:47
  • \$\begingroup\$ The for loop should also be if (!getline(cin, input) || input == "exit"). \$\endgroup\$ – David Sep 25 '16 at 18:55
28
\$\begingroup\$

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 << "\n\n";

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ A minor point: the behavior is implementation specific, not "implementation defined"; the latter is a term of art in the standard, and requires a compiler to document its behavior. \$\endgroup\$ – Pete Becker Sep 23 '16 at 12:45
  • 5
    \$\begingroup\$ When the C++ standard says that something is "Implementation defined" it means that a conforming implementation must document what it does. In the case of header files, C++ implementations are allowed to put other #include directives inside headers (which is different from C). So when you #include <iostream> you might get all the stuff from <string> as well. That's implementation-specific (or whatever other term you like). It's not implementation-defined, because the C++ standard does not require implementations to document their #include structure. \$\endgroup\$ – Pete Becker Sep 23 '16 at 19:22
  • 2
    \$\begingroup\$ "Don't using namespace std;" Why not? This advice is only valid for header files (or the files that are meant to be included). "Don't compare booleans to booleans" what do you want them to be compared to then? Perhaps you mean constants like true/false. \$\endgroup\$ – luk32 Sep 24 '16 at 14:24
  • 3
    \$\begingroup\$ @luk32 Not really. Possible name collisions can still happen in the source file. It also includes a lot of common names, string, list, vector, for_each, ... which are thus not available. Yes I meant that. \$\endgroup\$ – Rakete1111 Sep 24 '16 at 14:26
  • 4
    \$\begingroup\$ @luk32 I don't see how prefixing everything with std:: harms readability. If the a class defines for example a reverse function, and there is using namespace std; at the top of the source file, you'll get ambiguous functions. But yes, sometimes it is not necessary due to the coding style (e.g. every function starts with an upper case). \$\endgroup\$ – Rakete1111 Sep 24 '16 at 14:38
10
\$\begingroup\$

In addition to previous answers:

  1. You should check stream state after input (for example user can hit Ctrl+Z and enter or you can reach end of input file).
  2. return 0; is not necessary in the main function.
  3. Also you can move this code to the function and then replace break; with return;

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;
    }
}
\$\endgroup\$
7
\$\begingroup\$

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.

\$\endgroup\$
6
\$\begingroup\$

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;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Using SIGINT may be the standard for ending programs (although I've never seen it named so), but in general you'll have then to handle this signal, otherwise you'll quit without any cleanup. Also, it's implementation-specific, namely to POSIX. Not sure how an app on Windows can handle Control-C. Would be better to just say like "Enter the text <...> or press Enter to exit". \$\endgroup\$ – Ruslan Sep 24 '16 at 6:54
  • \$\begingroup\$ @Ruslan Yes, you can post it as another answer. \$\endgroup\$ – UniversallyUniqueID Sep 24 '16 at 8:56
4
\$\begingroup\$

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>
\$\endgroup\$
4
\$\begingroup\$

While you are starting you should eliminate as much bad practice as possible.

  1. Do not use namespace std; This is bad practice and will hurt you later on. So as soon as possible remove it and start adding std:: whenever needed.

  2. std::reverse is a general purpose routine, so I would suggest that you explore different ways to reach that goal.

    for (auto it = input.rbegin(); it != input.rend(); ++it) {
        std::cout << *it++;
    }
    std::cout << std::endl;
    

    Note that this piece of code will leave your string unaffected, whereas you current code actually inverts the string and then prints that out.

\$\endgroup\$
  • \$\begingroup\$ (My favourite compromise is the likes of using std::cout;.) \$\endgroup\$ – greybeard Sep 23 '16 at 8:28
  • \$\begingroup\$ I would not be surprised if cout ing the entire string once would be more efficient than cout ing the same string one character at a time. \$\endgroup\$ – coderodde Sep 23 '16 at 8:42
  • \$\begingroup\$ @coderodde That might be true, but the main reason to do it this way is that the data remains unchanged \$\endgroup\$ – miscco Sep 23 '16 at 9:03
  • \$\begingroup\$ I downvoted because you're using auto& it. You're lucky that any temporary can be converted to const ref, which is why referred to object is alive. Anyway, it should be auto it \$\endgroup\$ – Incomputable Sep 23 '16 at 11:35
  • 1
    \$\begingroup\$ @misco Why not edit the answer to fix the code? \$\endgroup\$ – Ruslan Sep 24 '16 at 7:01
3
\$\begingroup\$

The only tip I can give in a so short program is that you can eliminate the running boolean variable:

while(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)
    {
        break;
    }

    reverse(input.begin(), input.end());
    cout << "The reversed version of your text is: " << input << endl << endl;
}

Hope that helps.

\$\endgroup\$
2
\$\begingroup\$

Assuming you actually want user input to break out of your program rather than using Ctrl-C, you could do something like this:

#include<algorithm>
#include<iostream>
#include<string>

using std::cin;
using std::cout;
using std::endl;

int main() {
    while (true) {
             cout << "Enter the text you want to reverse: ";

             std::string in, answer;

             getline(cin, in);
             std::reverse(in.begin(), in.end());

             cout << "The reversed version of your text is: " << in << endl << endl;
             cout << "Would you like to reverse another string (y/n)? ";

             getline(cin, answer);

             if (answer.compare("n") == 0) {
                break;
             }
    }
}

I won't repeat what's been said about your basics, but rather have implemented them in this example.

\$\endgroup\$
2
\$\begingroup\$

This is not really necessary in this case, but if you're writing the reverse of a large string, then you shouldn't reverse it in place and then print. Rather print it directly:

std::copy(in.rbegin(), in.rend(),
   std::ostreambuf_iterator<char>(std::cout));

Using std::ostreambuf_iterator is to be preferred over using operator<< in a loop because the later suffers overhead from constructing a sentry object for each single character, whereas the former only needs a single sentry.

\$\endgroup\$
  • \$\begingroup\$ Have a look at miscco's answer. \$\endgroup\$ – greybeard Sep 24 '16 at 7:03
  • \$\begingroup\$ @greybeard I didn't saw that before, thanks for pointing it out. Still I think my answer is justified: it shows the only (?) way to do what's probably the simplest here: reverse printing of a string (in a single output operation) See my edit. \$\endgroup\$ – Daniel Jour Sep 24 '16 at 17:10

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.