Returning a Char Array from function in C

Started by
10 comments, last by Vortez 10 years, 6 months ago

If you can suggest an easier and\or better way of doing this please point me in the right direction. I'm only on my second build here and I know I have at least two more before I get all the features I want such as fragmentation (yea I know breaking files into parts is a bad idea [Microsoft] but I want the ability if it is needed)


Sure;
  • Always* keep the null-terminator in mind when dealing with strings: ret isn't large enough to hold 16 characters and the null terminator; sprintf is writing the null terminator in memory that doesn't belong to ret!
  • Always* look for ways to reduce and refactor duplicate code: Notice the format parameter for sprintf in BinHex is twice that of bytes? You can reduce this whole case statement by adding another level of indirection; try using another sprintf to prepare the format parameter. The switch case in HexBin can also be reduced to a loop.
  • Always* enforce parameter constraints. The bytes parameter can only be between 1 and 8, but you have a special behavior defined when it is not. It may be better to throw a fatal error in this case by using assert.
*Be wary of statements such as "always do this" or "never do that", it's "always" situational and may not fit every use case.

Here's the reduced code:
[spoiler]



#include <assert.h>

// 17 characters, to make room for the null-terminator
void BinHex(char Bin[17], char ret[17], int bytes) {
	// Enforce constraints
	assert( 1 <= bytes && bytes <= 8 );

	// Here, we use pointers to read and write to and from ret and Bin respectively
	char* dst = &ret[0];
	char* src = &Bin[0];

	// Equivalent to "for ( ; bytes > 0; bytes-- )"
	while ( bytes-- ) {
		// sprintf returns the number of characters written, so can use this
		// to our advantage and move the write pointer 2 characters for 
		// every 2 characters written
		dst += sprintf(dst,"%02x",(unsigned int)(*src++) );   // "*src++" read a charater from src and moves it to the next character
	}
}

void DexHex(unsigned int Dec, char ret[17], int bytes) {
	assert( 1 <= bytes && bytes <= 8 );

	char fmt[5];
	// "%%" -> "%"
	// "%02i" -> zero-padded bytes*2, width of 2
	// "x" -> "x"
	sprintf(fmt,"%%%02ix",bytes*2); // When bytes=4 and Dec=65534, "%%%02ix" -> "%08x"
	sprintf(ret,fmt,Dec);  // "%08x" -> 0000FFFE
}
[/spoiler]

Hope this helps!

EDIT: Formatting/editing

That helped a lot, thanks. I did have to edit your code to the following BinHex was slightly wrong:


#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <assert.h>
#include "MyFS.h"


// 17 characters, to make room for the null-terminator
void BinHex(char Bin[17], char ret[17], int bytes)
{
    // Enforce constraints
    assert( 1 <= bytes && bytes <= 8 );


    // Here, we use pointers to read and write to and from ret and Bin respectively
    char* dst = &ret[0];
    char* src = &Bin[0];


    // Equivalent to "for ( ; bytes > 0; bytes-- )"
    while ( bytes-- )
    {
        // sprintf returns the number of characters written, so can use this
        // to our advantage and move the write pointer 2 characters for 
        // every 2 characters written
        dst += sprintf(dst,"%02x",(unsigned int)(src[bytes]) );   // "src[bytes]" read a charater from src and moves it to the previous character
    }
}


void DecHex(unsigned int Dec, char ret[17], int bytes)
{
    assert( 1 <= bytes && bytes <= 8 );


    char fmt[5];
    // "%%" -> "%"
    // "%02i" -> zero-padded bytes*2, width of 2
    // "x" -> "x"
    sprintf(fmt,"%%%02ix",bytes*2); // When bytes=4 and Dec=65534, "%%%02ix" -> "%08x"
    sprintf(ret,fmt,Dec);  // "%08x" -> 0000FFFE
}


unsigned int HexDec(char* Hx)
{
    unsigned int ret;
    ret = (unsigned int)strtol(Hx, NULL, 16);
    return ret;
}

But it defiantly helped with readability

Thanks,

Brian T Hoover

Advertisement

unsigned int HexDec(char* Hx)
{
    unsigned int ret;
    ret = (unsigned int)strtol(Hx, NULL, 16);
    return ret;
}

All this can be reduced to:


UINT HexDec(char *Hx)

{

    return strtol(Hx, NULL, 16);

}

Also...


void BinHex(char Bin[17], char ret[17], int bytes)
{
    // Enforce constraints
    assert( 1 <= bytes && bytes <= 8 );


    // Here, we use pointers to read and write to and from ret and Bin respectively
    char* dst = &ret[0];
    char* src = &Bin[0];
...
}

This just look wrong to me and should be declared like this:


void BinHex(char *src, char *dst, int bytes)
{
...
}

// And used like this...
char Bin[17];
char ret[17];

BinHex(&Bin[0], &ret[0], some_number);


Still, it look a little better than before, keep it up.

This topic is closed to new replies.

Advertisement