Returning a C string from a function in C

Question

I have written a function in c to convert a base-10 number into its binary representation with 16 bits. A space should also appear in the output of this function, E.G.: 00000000 00000001 = 1. The conversion itself works correctly, but I'm having trouble getting this value back to the main as a string. I don't get any errors, but upon printing sequence_number (one character at a time), I get ASCII symbols. I realize that this is common question, but I have read many similar posts and cannot identify what I have done wrong.

void convertToBinary(char *ReturnV, int _this){
    //Declare Variables
    int bin_no[16]={0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0}, bin_Index;
    int working = _this;
    int i, c, singleDigit, a;
    char working_Return[19];
    char binaryDigit[1];

    for(bin_Index = 15; bin_Index > 0; bin_Index--) {//Conversion
        working = (working/2);
        bin_no[bin_Index] = working%2;
    }

    for(i = 0; i < 17; i++) {//Convert to string
        singleDigit = bin_no[i];
        sprintf(binaryDigit, "%d", singleDigit);
        working_Return[i] = binaryDigit[0];
    }

    for(a = 17; a > 9; a--) {//Insert space
        //Copy all values over
        working_Return[a+1] = working_Return[a];
    }

    working_Return[9] = ' ';//Insert Space
    strcpy(ReturnV, working_Return);    
}    

My function is called using

int sequenceNumber_Lower = 48;
char sequence_number[19];
convertToBinary(sequence_number, sequenceNumber_Lower);

but when I attempt to print the values from sequence_number (from main) using

for(c=0 ; c<18 ; c++) { 
    printf("%c", sequence_number[c]);
} 

I get random ascii symbols. I've verified that the working_Return string holds the correct values so the error must lie in copying the values at the end of the function. Or have I done this incorrectly and I am trying to print what has been deleted?

I'm aware that when the function ends, the local variables are destroyed, but I feel that I've done this correctly as I've looked at many posts on here and other sites that say this is one of the valid ways of returning a string from a function. Can you help me see what I've done wrong?

I've tried returning a pointer to this string with the function declaration (and according definition)

char * convertToBinary(int _this);

But this had the same results.

Answer 1

This is your code after few fixes (descibed below):

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

void convertToBinary(char *ReturnV, int _this){
    int bin_no[16]={0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0}, bin_Index;
    int working = _this;
    int i, singleDigit, a;
    char binaryDigit[1];

    for(bin_Index = 15; bin_Index >= 0; bin_Index--) {
        working = (working/2);
        bin_no[bin_Index] = working%2;
    }

    for(i = 0;  i < 18; i++) {
        if(i<=7)
            ReturnV[i] = (char)(((int)'0')+bin_no[i]);
        else if(i==8)
            ReturnV[8] = ' ';
        else
            ReturnV[i] = (char)(((int)'0')+bin_no[i+1]);
    }
    ReturnV[17] = '\0';
}    

int main()  {
    int sequenceNumber_Lower = 48;
    char sequence_number[18];
    convertToBinary(sequence_number, sequenceNumber_Lower);
    printf("%s\n", sequence_number);
}

Fixes:

1. Instead of using sprintf(), use (char)(((int)'0')+bin_no[i]) to convert int to a char.
2. Save directly to the ReturnV string, thus avoiding the need to create another string.
3. Allocate arrays only as long as you need them to be.
4. Add \0 at the end.

Answer 2

There are a number of errors, and some ineffiencies too. Mainly, the number of loop iterations are a bit erratic, and the sprintf() statement is writing an integer number as text into a string only 1 char long - that will never work.

The inefficiencies are that you can use the function arguments directly. int _this can be used in exactly the same way as the local variable int working. You can also write your result directly into the string char *ReturnV passed (provided it is long enough).

#include <stdio.h>
#include <string.h>

void convertToBinary(char *ReturnV, int working){
    int bin_Index;                    // remove most of the vars
    for (bin_Index = 16; bin_Index > 0; bin_Index--) {  // skip 1st string char
        ReturnV[bin_Index] = '0' + (working & 1);       // ascii text
        working >>= 1;
    }
    ReturnV [17] = 0;                 // terminate string
    strncpy(ReturnV, ReturnV+1, 8);   // create a gap
    ReturnV [8] = ' ';                // Insert Space
}    

int main () {
    int sequenceNumber_Lower = 48;
    char sequence_number[19];
    convertToBinary(sequence_number, sequenceNumber_Lower);
    printf("%s", sequence_number);
    return 0;
}

Answer 3

Your code has two main problems:

You use sprintf in an string with only one char, but sprintf always puts an '\0' after the last char printed, so it would overwrite memory outside your string. This is called buffer overrun.

Another problem in your code is that in C all strings passed to printf MUST end with '\0'. It is the marker of "End of String" used by default in C. If your string does not have one, the printf function will continue printing whatever is in memory until it find one.

So declaring

char binaryDigit[2];

and in the end adding:

working_Return[18] = '\0';

will solve your problems.

The changes proposed on other answers are valid as well. For example:

'0' + (working & 1)

is much faster than sprintf. Writing directly to the buffer is also much faster.

Answer 4

In addition to other problems, you are not null-terminating your working string before strcpy, You must remember to ALWAYS INITIALIZE YOUR VARIABLES AND TERMINATE YOUR STRINGS:

char working_Return[19];

should be:

char working_Return[19] = {0};

This will null-terminate your working string (i.e. '0' following any character placed in the string beginning at working_Return[0] up to and including working_Return[18]) unless you overwrite it and it will allow strcpy to find the end of the string (preventing all those funny characters). As for the binary conversion, there are several ways to approach it. One of my favorite is:

#include <stdio.h>

/** returns pointer to formatted binary representation of 'n' zero padded to 'dsz'.
 *  returns pointer to string contianing formatted binary representation of 
 *  unsigned 64-bit (or less ) value zero padded to 'dsz' digits with char
 *  'sep' placed every 'gsz' digits. (e.g. 10001010 -> 1000-1010).
 */
char *fmt_binstr (unsigned long n, unsigned char dsz, unsigned char gsz, char sep) {

    static char s[128 + 1] = {0};
    char *p = s + 128;
    unsigned char i;

    for (i = 0; i < dsz; i++) {
        p--;
        if (i > 0 && gsz > 0 && i % gsz == 0)
            *p-- = sep;
        *p = (n >> i & 1) ? '1' : '0';
    }

    return p;
}

int main () {

    printf ("\n  Usage:\n\n  char *fmt_binstr (unsigned long number,\n\t\t    unsigned char display_size,\n\t\t    unsigned char grouping_size,\n\t\t    char group_seperator)\n\n");
    printf ("\nExample of formatted binary strings\n\n");
    printf ("  fmt_binstr (253, 8, 0, 0)            :  %s\n", fmt_binstr (253, 8, 0, 0));
    printf ("  fmt_binstr (253, 8, 2, '-')          :  %s\n", fmt_binstr (253, 8, 2, '-'));
    printf ("  fmt_binstr (253, 8, 4, '-')          :  %s\n", fmt_binstr (253, 8, 4, '-'));
    printf ("  fmt_binstr (253, 16, 4, '-')         :  %s\n", fmt_binstr (253, 16, 4, '-'));
    printf ("  fmt_binstr (253, 16, 2, ':')         :  %s\n\n", fmt_binstr (253, 16, 2, ':'));

    printf ("  fmt_binstr (0xbeefcafe, 32, 0, 0)    :  %s\n", fmt_binstr (0xbeefcafe, 32, 0, 0));
    printf ("  fmt_binstr (0xbeefcafe, 32, 8, '-')  :  %s\n", fmt_binstr (0xbeefcafe, 32, 8, '-'));
    printf ("  fmt_binstr (0xbeefcafe, 32, 4, '-')  :  %s\n\n", fmt_binstr (0xbeefcafe, 32, 4, '-'));

    return 0;
}

output:

$ ./bin/prnbin

  Usage:

  char *fmt_binstr (unsigned long number,
                    unsigned char display_size,
                    unsigned char grouping_size,
                    char group_seperator)


Example of formatted binary strings

  fmt_binstr (253, 8, 0, 0)            :  11111101
  fmt_binstr (253, 8, 2, '-')          :  11-11-11-01
  fmt_binstr (253, 8, 4, '-')          :  1111-1101
  fmt_binstr (253, 16, 4, '-')         :  0000-0000-1111-1101
  fmt_binstr (253, 16, 2, ':')         :  00:00:00:00:11:11:11:01

  fmt_binstr (0xbeefcafe, 32, 0, 0)    :  10111110111011111100101011111110
  fmt_binstr (0xbeefcafe, 32, 8, '-')  :  10111110-11101111-11001010-11111110
  fmt_binstr (0xbeefcafe, 32, 4, '-')  :  1011-1110-1110-1111-1100-1010-1111-1110

You can use 64-bit values as well (just a bit long for the example).