# Take all the bits in a number and shift them to the left end

I wrote this program which takes all the bits in a number and shifts them to the left end (for example: 01010110 --> 11110000). It's and exercise from a book. It works, but it seems to be very wasteful to use integers (I only need 8 bits).

Can you tell me if the code is proper or not? Is the algorithm good?

#include <iostream>

void print_bit(const int bit)
{
for(int i = 0x80; i != 0; i >>= 1)
{
if((bit & i) != 0)
std::cout << "1";
else std::cout << "0";
}
std::cout << std::endl;
}

int set_bits(const int bit)
{
int lefted = 0xFF;
for(int i = 0x80; i != 0; i >>= 1)
{
if((bit & i) == 0)
lefted <<= 1;
}
return lefted;
}

int main()
{
int bit = 11;
print_bit(bit);
print_bit(set_bits(bit));
return 0;
}

• It's a good algorithm. The code is not bad. I agree with most of what Jamal says, but I don't like combining the if with its body on a single line. The standard puts a space between the keywords like for and if and the open parenthesis; you should too. Your print_bit() function should not include the newline; it makes it hard to use in a more general case, such as a loop ranging from 0 to 255 (inclusive) and printing 4 pairs of entries per line of output. It would be better to have the printing function just output the binary value and then have the calling code do the punctuation. – Jonathan Leffler Sep 4 '13 at 2:54
• @JonathanLeffler: Good point about the single line thing. Although I think using a single line is okay at times, this may not be one of them. I'll change that, along with some other things. – Jamal Sep 4 '13 at 5:30

It appears the correct results are given, which is good. I do see some miscellaneous things, though.

• The naming and execution in main() seem confusing. Try something like this:

int main()
{
// the name "bit" is misleading; you're using an integer
// name it something more accurate (this is just an idea)
int original = 11;

// why not return the final (shifted) integer?
// sticking set_bit() into print_bit() is hard to read
int final = set_bits(original);

// print both integers
print_bit(original);
print_bit(final);
}

• This:

if((bit & i) != 0)
std::cout << "1";
else std::cout << "0";


could become a single-line ternary statement:

// read as: (statement is true?) ? (if so, do this) : (if not, do this)
std::cout << ((bit & i) != 0) ? "1" : "0";

• This:

if((bit & i) == 0)
lefted <<= 1;


could have curly braces (to allow any additional code for the body):

if ((bit & i) == 0)
{
lefted <<= 1;
}

• I'd probably make your "magic numbers" (such as 0x80) constants. This could help provide more context for your code, especially when there's no relevant documentation.

• std::cout << std::endl; doesn't need to be in print_bit(). The display function should only print the values; that's what the user expects. If you want a newline with the displaying, put it around the calling code. That way, the displaying will be easier to adjust.