Testing Bits To Create A String - Is there a better approach?

Question

This code works, but I'm wondering if there is a better way to do it. Basically I need to test bits, and write the appropriate character or characters to a string depending on the state of the bit. The spaces are present because the characters will be displayed with a fixed width font and I'd like to keep them from moving around. C or C++ is fine.

const char* Letters[10] = {"A", "B", "Sl", "St", "R", "L", "U", "D", "RS", "LS"};
const char* Ext[2] = {"X", "Y"};
const char* Spaces[10]  = {" ", " ", "  ", "  ", " ", " ", " ", " ", "  ", "  "};

char str[60];
char FinalString[60];

void MakeBitString(u16 data, u16 dataExt) {

    int x;
    strcpy(str, "");

    for (x = 0; x < 2; x++) {

     //X and Y
     if(dataExt & (1 << x)) {
      strcat(str, Spaces[x]); 
     }
     else
      strcat(str, Ext[x]);
    }

    for (x = 0; x < 10; x++) {

     //the rest
     if(data & (1 << x)) {
      strcat(str, Spaces[x]); 
     }
     else
      strcat(str, Letters[x]);
    }

    strcpy(FinalString, str);
}

Answer 1

• use std::string instead char* and strcat;
• why you need array with spaces? it seems could be just one space;
• you have almost indentical code for two u16 parameters - make one little function and call it twice;
• don't write result in global variable - return std::string

Answer 2

At the cost of some dynamic allocations (inside std::string), you can make this code more easily modifiable by not having any hard-coded numbers:

#define ARRAYSIZE(A) (sizeof(A)/sizeof((A)[0]))

std::string MakeBitString(u16 data, const std::string* letters, int count) {
    std::string s;
    for (int x = 0; x < count; x++) {
        if (data & (1 << x))
            s.append(letters[x].size(), ' '); 
        else
            s += letters[x];
    }
    return s;
}

std::string MakeBitString(u16 data, u16 dataExt) {
    const std::string Letters[] = {"A", "B", "Sl", "St", "R", "L", "U", "D", "RS", "LS"};
    const std::string Ext[] = {"X", "Y"};

    std::string s = MakeBitString(dataExt, Ext, ARRAYSIZE(Ext));
    s += MakeBitString(dataExt, Letters, ARRAYSIZE(Letters));
    return s;
}

Answer 3

That should be fine; if you want to add buttons or axes, though, you might want to generalize it a bit.

Answer 4

I recommend something a little more explicit, which doesn't use loops, because you appear to have only a small number of bits to check. If this needs to scale to tens of thousands of bits, then by all means use loops.

I'm also assuming that you have a great reason for using global variables and fixed-length character arrays.

Here is what I would do:

char FinalString[60];

void ConcatBitLabel(char ** str, u16 data, u16 bitMask, const char * label)
{
    if (data & bitMask)
    {
        // append spaces for strlen(label)
        while (*label) { *((*str)++) = ' '; label++; }
    }
    else
    {
        // append the label
        while (*label) { *((*str)++) = *label; label++; }
    }
}

void MakeBitString(u16 data, u16 dataExt)
{
    char * strPtr = FinalString;

    ConcatBitLabel(&strPtr, dataExt, 0x0001, "X");
    ConcatBitLabel(&strPtr, dataExt, 0x0002, "Y");

    ConcatBitLabel(&strPtr, data, 0x0001, "A");
    ConcatBitLabel(&strPtr, data, 0x0002, "B");
    ConcatBitLabel(&strPtr, data, 0x0004, "Sl");
    ConcatBitLabel(&strPtr, data, 0x0008, "St");
    ConcatBitLabel(&strPtr, data, 0x0010, "R");
    ConcatBitLabel(&strPtr, data, 0x0020, "L");
    ConcatBitLabel(&strPtr, data, 0x0040, "U");
    ConcatBitLabel(&strPtr, data, 0x0080, "D");
    ConcatBitLabel(&strPtr, data, 0x0100, "RS");
    ConcatBitLabel(&strPtr, data, 0x0200, "LS");

    *strPtr = 0; // terminate the string
}

Answer 5

Basically the C++ solution looks like

Codes convert( std::size_t data,
               const Codes& ext, 
               const Codes& letters )
{
    Codes result;
    std::transform( ext.begin(),
                    ext.end(),
                    std::back_inserter( result ),
                    Converter( data ) );

    std::transform( letters.begin(),
                    letters.end(),
                    std::back_inserter( result ),
                    Converter( data ) );
    return result;
}

Where Converter is implemented like

struct Converter
{
    Converter( std::size_t value ):
        value_( value ), x_( 0 )
    {}
    std::string operator() ( const std::string& bitPresentation )
    {
        return ( value_ & ( 1 << x_++ ) ) ?
            std::string( bitPresentation.size(), ' ' ):
            bitPresentation;
    }
    std::size_t value_;
    std::size_t x_;
};

Here is the convert from Codes to string function

std::string codesToString( const Codes& codes )
{
    std::ostringstream stringStream;
    std::copy( codes.begin(), codes.end(), 
               std::ostream_iterator<std::string>( stringStream ) );
    return stringStream.str();
}

Answer 6

Here's one bit-twiddly way to do it in a single pass. It's even extendable to up to 16 bits, as long as you ensure the wide mask has a bit set anywhere you have a 2-character signifier.

#define EXT_STR "XY"
#define DATA_STR "ABSlStRLUDRSLS"
const char FullStr[] =  EXT_STR DATA_STR;
#define  EXT_SZ  2 //strlen(EXT_STR);

void MakeBitStr(u16 data, u16 dataExt) {
    char* dest = FinalString;
    const char* src= FullStr;
    u16 input = (data<<EXT_SZ)|dataExt;
    u16 wide = (0x30C<<EXT_SZ)|0;  //set bit for every 2char tag;
    while ((src-FullStr)<sizeof(FullStr))
    {   *dest++ = (input&1)?' ':*src;
        if (wide&1)
        { wide&=~1;
        }
        else
        { input>>=1;wide>>=1;
        }
        src++;
    }
    *dest='\0';
}

Answer 7

Non-hacky, clean solution:

std::string MakeBitString(u16 data, u16 dataExt) {
    std::string ret;

    static const char *letters = "A B SlStR L U D RSLS";
    static const char *ext = "XY";
    static const char *spaces = "  ";

    for(int bit = 0; bit < 2; ++bit) {
        const char *which = (dataExt & 1) ? &ext[bit] : spaces;

        ret += std::string(which, 0, 1);

        dataExt >>= 1;
    }

    for(int bit = 0; bit < 10; ++bit) {
        const int length = letters[bit * 2 + 1] == ' ' ?  1 : 2;
        const char *which = (dataExt & 1) ? &letters[bit * 2] : spaces;

        ret += std::string(which, 0, length);

        dataExt >>= 1;
    }

    return ret;
}