## Question:

I am writing a code to find the number of ones in binary representation of a numberhere is the code:

```
int main(void)
{
int n,tNum,count = 0;
cin >> n;
tNum = n;
while(tNum > 0)
{
int i = 0;
int bit = getBit(n,i);// get bit
if (bit == 1)
{
count++;
}
tNum >> 1;
i++;
}
cout << count << endl;
}
```

Above code gives an endless loop and tNum didn’t change its value
I didn’t understand where I am doing wrong## Answer:

Bitwise right shift operator >> didn’t worked as intended

The bitwise right shift operator

`>>`

is working as intended but you are not storing the result of `>>`

operator anywhere. This expression```
tNum >> 1;
```

will right shift the value of `tNum`

by `1`

but the result of this expression is unused.Use

`>>=`

operator instead of `>>`

. It should be:```
tNum >>= 1;
```

This is same as `tNum = tNum >> 1`

.Another problem in your code:

This

```
getBit(n,i);
```

will give the `0`

^{th}bit of

`n`

(as the `i`

is initialised with `0`

in every iteration) and which is going to be same for every iteration. Instead, you should get the `0`

^{th}bit of

`tNum`

as you are using shift operator on `tNum`

below in your code. Also, you don’t need `i`

at all, just remove it. The statement should be:```
int bit = getBit(tNum, 0);// get 0th bit
```

One more important point, the result of right shift `>>`

of a negative number is implementation defined. May, you should use `unsigned int`

type for `n`

and `tNum`

.You can do:

```
int main (void)
{
unsigned int n, tNum;
int count = 0;
cin >> n;
tNum = n;
while(tNum > 0)
{
int bit = getBit(tNum, 0);// get bit
// You can also do
// int bit = tNum & 1;
if (bit == 1)
{
count++;
}
tNum >>= 1;
}
cout << count << endl;
return 0;
}
```

Note: There is scope of improvement in the implementation of your code. I am leaving it up to you to identify those improvements and make respective changes in your code.If you have better answer, please add a comment about this, thank you!

Source: Stackoverflow.com

## Leave a Review