2
\$\begingroup\$

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;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – Jonathan Leffler Sep 4 '13 at 2:54
  • \$\begingroup\$ @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. \$\endgroup\$ – Jamal Sep 4 '13 at 5:30
2
\$\begingroup\$

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.

\$\endgroup\$

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.