Simple text-to-binary converter written in C++

Question

I am learning C++ on my own and was hoping for some feedback on this simple text-to-binary converter. The program runs and I haven't encountered any errors with it yet. Any input is welcome.

#include <iostream>
#include <string>
#include <bitset>
using namespace std;
int main()
{
char letter = ' ', playAgain = 'y';
string word = " ";
cout << "\t**Text To Binary Convertor**\n\n";
while (playAgain == 'y'){
 cout << "Please enter a character, word, or phrase: ";
 getline (cin, word, '\n');
 cout << "\nThe binary value for " << word << " is \n";
 for (unsigned int wordPosition = 0; wordPosition < word.size(); ++wordPosition){
 letter = word[wordPosition];
 bitset <8> binary(letter);
 cout << binary;
 }
 cout << "\n\nWould you like to try again? (y/n)";
 cin >> playAgain;
 if (playAgain != 'y'){
 cout << "\n\nExiting program.";
 playAgain = 'n';
 }
 cin.ignore();
}
return 0;
}

Answer 1

I don't see anything obviously wrong with the code, so that's a good start.

You're using a very unusual brace indentation style. Wikipedia calls it Banner Style but I don't think I've ever seen it before.

You're checking for playAgain != 'y' in two different places, which is redundant and could lead to bugs if one of them is modified. I'd move that whole second block outside of the main loop.

• Guy: Thanks - I'm not sure where the indentation style I am using came from. I've been using a number of books from the library and on-line sources. Now that I look at the books again, they don't use that style. weird.

Answer 2

As already mentioned, your code is looking okay. Here are my additions:

• It's best not to use using namespace std.

• Variables should be declared/initialized on their own line:

  char letter = ' ';
  char playAgain = 'y';
  

• You don't need a newline between every line of code. This will just make your program appear longer and harder to read. Just separate lines into groups based on purpose.

• Variable declarations/initializations should be placed as close in scope as possible to where they will be first used:

  std::cout << "Please enter a character, word, or phrase: ";
  std::string word; // declaration is first used here
  getline(std::cin, word, '\n');
  

• Your main loop could use for (;;). This is essentially an infinite loop, but it should do a break at the end if the user inputs a value other than 'Y':

  for (;;)
  {
   // do stuff
   std::cout << "\n\nWould you like to try again? Y/N)";
   char playAgain;
   playAgain = std::toupper(playAgain);
   std::cin >> playAgain;
   std::cin.ignore();
   if (playAgain != 'Y')
   {
   break;
   }
   // playAgain is 'y', so do stuff again
  }
  

• Prefer to use std::string::size_type for the inner for-loop as it's the type returned from std::string::size().

  Or if you have C++11, use a range-based for loop instead:

  for (auto iter& : word)
  {
   letter = iter;
   // ...
  }
  

• Tim Seguine: anything wrong with for(char playAgain='y';playAgain=='y';std::cin >> playAgain,std::cin.ignore()) I think there is something to be said for putting the control flow up at the top, since that is what it is for.

Answer 3

Your code looks good overall. Just a couple comments...

When writing C++ code, you generally use "endl" instead of "\n" to terminate lines.

cout << "hi" << endl;

Also, to print out the ascii value of the character you don't necessary the "char" value into another variable. In your case, you are copying it twice. You can print an ascii value directly from the string by using a cast.

cout << (unsigned short)word[wordPosition] << endl;

• Lightness Races in Orbit: "\n" to terminate lines is just fine. It speeds things up too by not forcing needless flushes. And your cast is more-or-less equivalent to the original example with a named variable. Casts copy.
• Ben Voigt: And the compiler should get rid of the extra copy. Plus I don't think he's printing ASCII values numerically, I think he's printing them in base-2 (individual bits)
• Guy: @Ben Thanks for the code review site. And thanks to all for your help. Sorry about this misplaced post.
• Jamal: -1 for the same reasons as Lightness.