Mfrhtyj

This program converts a decimal number to a binary number. This is one of my first C programs and I am wondering if I have used the elements of this language properly. Suggestions for improvement are welcome.

#include <stdio.h>

#include <string.h>



void print_out_reversed(char string)

{

    int index = strlen(string);



    while (string[index] != '')

        index--;



    for (int i = index; i >= 0; i--)

        putchar(string[i]);

    putchar('n');

}



void print_decimal_number_binary(int number)

{

    if (number == 0)

    {

        printf("0n");

        return;

    }



    char bits[sizeof(int) * 8 + 1] = {0};

    int index = 0;



    while (number > 0)

    {

        if (number % 2 == 0)

        {

            bits[index] = '0';

        }

        else

        {

            bits[index] = '1';

        }

        number = number / 2;

        index++;

    }



    print_out_reversed(bits);

}



int main()

{

    printf("enter number: ");

    int number;

    scanf("%i", &number);

    print_decimal_number_binary(number);

}

Terminology

It's important to be able to understand (and describe) what's actually going on. Your program

1. converts from an integer decimal string representation to an integer using scanf. This integer is then represented as a binary number in the processor.


2. converts from that integer back into a string representation, but rather than it being decimal, it's binary.

So yes - it technically converts from "decimal to binary", but really it's "decimal string to integer to binary string".

Use const

void print_out_reversed(char string)

doesn't modify string, so write const char string.

Simplify your strlen usage

This:

int index = strlen(string);



while (string[index] != '')

    index--;



for (int i = index; i >= 0; i--)

    putchar(string[i]);

can be

for (int i = strlen(string)-1; i >= 0; i--)

    putchar(string[i]);

It seems that you don't trust what strlen is doing, which is why you have that intermediate while loop. But that loop won't have any effect, because the null terminator will always be where strlen says it is.

Use math instead of if

This:

    if (number % 2 == 0)

    {

        bits[index] = '0';

    }

    else

    {

        bits[index] = '1';

    }

can be

bits[index] = '0' + (number & 1);

Use combined operation and assignment

This:

number = number / 2;

should be

number /= 2;

or, for speed (which the compiler will do for you anyway)

number >>= 1;

#include <stdio.h>



void get_bits(void * num, char * out, int bytes);



int main(void)

{

    int x = 0;



    printf("Enter an integer: ");

    scanf("%i", &x);



    char bits[65] = {0};//length of 65 here, but only need length of 33

    get_bits(&x, bits, 4);



    printf("%d in binary %sn", x, bits);



    return 0;

}



//assumes char array of length 1 greater than 

//number of bits

//EDIT: VERSION WITHOUT MEMCPY AS REMINDED BY @REINDERIEN

void get_bits(void * num, char *out, int bytes)

{

    unsigned long long filter = 0x8000000000000000;

    unsigned long long *temp = num;



    if(bytes <= 0) return;

    if(bytes > 8) bytes = 8;



    filter = filter >> (8*(sizeof(unsigned long long)-bytes));

    //memcpy(&temp, num, bytes);

    int bits = 8*bytes;

    for(int i=0;i<bits;i++) {

        //if(filter & temp)

        if((filter >> i) & *temp)

            out[i] = '1';

        else

            out[i] = '0';



        //temp = temp << 1;

    }

    out[bits] = '';

}

EDIT

Sorry, I just realized what Code Review meant. I figured I was suggesting an improvement, but I see how it did not match the intent here. Maybe this is closer.

Improvements

The posted code requires several loops, divisions, and modulus calculations. While it does solve the problem of representing an integer in binary, the utility may be limited by additional clock cycles.

The code may be optimized and extended to use with other integer representations, including char, short, or long long (or long depending on the size of long).

One drawback of the posted code is the need to reverse bits. Utilizing a mask to filter which bits are set in the number is more efficient.

Alternative Solution

The function get_bits will accept any integer representation (presumably floating-point numbers as well, though I have not used it for this and not tested it).

It will "return," really populate, a character array with up to a 64-bit bit representation of the number.

It NO LONGER relies on memcpy from string.h.

Inputs for get_bits

void *num : a pointer to the memory address of the number to be represented in
binary

char *out : the address of a character array to store the bit representation.

NOTE: This should be of length 1 longer than the number of bits to
be represented

int bytes : number of bytes containing the number to represent in binary

Implementation

Based on the size of the data type of the number to be represented, a mask is established with the highest bit set. This is the variable, filter, of type unsigned long long contained in 64-bits. The input number, as a void*, is effectively cast to a pointer to unsigned long long a stored in temp. Using bit shifting, a time requirement of 1 clock cycle, the filter is shifted to the right to align it with the highest bit of the number.

Ex. In hexadecimal, a 16-bit filter would be 0x8000, which in binary is 100000000000000.

Only a single for loop is performed to populate the output string. In each iteration of the loop, a bit-wise AND is performed with filter and *temp. The result of this expression is either 0 or non-zero. The result is 0 only, when the highest order bit of temp is 0. The position in the output string is set to 1 if non-zero or 0 otherwise.

At the end of each iteration the filter is shifted incrementally by 1 more bit to the right.

Ex. In binary, if temp is 1010, then temp << 1 is 0100. (a suitable filter would be 1000 in binary)

I'm adding another answer in a different direction from my previous one, both to show the OP some alternative techniques, and to illustrate an adaptation of @RJM's method.

Here's the code; it's quite simple:

#include <stdint.h>

#include <stdio.h>



static void printBinary(const uint8_t *restrict num, int bytes) {

    for (int p = bytes - 1; p >= 0; p--) {

        uint8_t x = num[p];

        for (int i = 0; i < 8; i++) {

            putchar('0' | (x >> 7));

            x <<= 1;

        }

    }

}



int main() {

    int64_t x;

    for (;;) {

        puts("Enter an integer: ");

        if (scanf("%lld", &x) == 1)

            break;

        while (getchar() != 'n');

    }



    printBinary((uint8_t*)&x, sizeof(x));

    putchar('n');



    return 0;

}

Things to observe as compared to the OP's code (and RJM's code):

• There are no calls to malloc or memcpy.


• The result is not stored in memory; it's output directly to stdout.


• The input integer has a primitive form of validation. The program will loop until scanf succeeds.


• The input integer supports 64 bits instead of 32.


• The output routine supports integers of any length, with the assumption that the integer is little-endian.


• The bit sequence reversal is done directly in the decoding loop, rather than as a separate step.


• There is no need for a "filter" (mask) variable.


• The main loop does not need an if.

I'm happy to explain any aspect of this approach for the purposes of education.

//          Sergio's   

 #include <stdio.h>

 #include <stdlib.h>

 #include <string.h>

 void binary(unsigned number) 

  { 

  if (number > 1)



  binary(number/2);

  printf("%d", number  % 2);

  } 

  int  main(void)

    {

       int value=10;

       binary(value);

       return 0;

     }