HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

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

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
shiftleftnumberthealltakebitsendandthem

Problem

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 

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

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

Solution

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

Code Snippets

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);
}
if((bit & i) != 0)
std::cout << "1";
else std::cout << "0";
// read as: (statement is true?) ? (if so, do this) : (if not, do this)
std::cout << ((bit & i) != 0) ? "1" : "0";
if((bit & i) == 0)
    lefted <<= 1;
if ((bit & i) == 0)
{
    lefted <<= 1;
}

Context

StackExchange Code Review Q#30740, answer score: 3

Revisions (0)

No revisions yet.