7
\$\begingroup\$

I have function which writes data of a specified type to a buffer. Here is the part of this which writes Uint8, Uint16, Uint32 and Uint64, in big and little endian. As you can see that the same code is repeated several times, so I want to make this code more elegant.

...        
            case BW_DATA_TYPE_UINT8:
            {
                Uint8_T val = *(static_cast(Uint8_T*, src));
                Uint8ToLittleEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] );
                bw->curPos += length;
                break;
            }

            case BW_DATA_TYPE_UINT16_LE:
            {
                Uint16_T val = *(static_cast(Uint16_T*, src));
                Uint16ToLittleEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + 1 );
                bw->curPos += length;
                break;
            }

            case BW_DATA_TYPE_UINT32_LE:
            {
                Uint32_T val = *(static_cast(Uint32_T*, src));
                Uint32ToLittleEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + 3 );
                bw->curPos += length;
                break;
            }

            case BW_DATA_TYPE_UINT64_LE:
            {
                Uint64_T val = *(static_cast(Uint64_T*, src));
                Uint64ToLittleEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + 7 );
                bw->curPos += length;
                break;
            }


            case BW_DATA_TYPE_UINT16_BE:
            {
                Uint16_T val = *(static_cast(Uint16_T*, src));
                Uint16ToBigEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + 1 );
                bw->curPos += length;
                break;
            }

            case BW_DATA_TYPE_UINT32_BE:
            {
                Uint32_T val = *(static_cast(Uint32_T*, src));
                Uint32ToBigEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + 3 );
                bw->curPos += length;
                break;
            }

            case BW_DATA_TYPE_UINT64_BE:
            {
                Uint64_T val = *(static_cast(Uint64_T*, src));
                Uint64ToBigEndianAr( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + 7 );
                bw->curPos += length;
                break;
            }
    ...
\$\endgroup\$
6
  • 2
    \$\begingroup\$ This is C++ code not C. RTTI casts are only available in C++. \$\endgroup\$
    – Lundin
    Sep 22, 2011 at 14:18
  • \$\begingroup\$ @Lundin: static_casts aren't available in C -- but they don't use RTTI. RTTI would only get involved if you used dynamic_cast. \$\endgroup\$ Sep 22, 2011 at 22:06
  • \$\begingroup\$ @Lundin: It is not C++ static_cast<>, it is macros which I defined by my own, to use instead of old style type conversations. \$\endgroup\$
    – akmal
    Sep 23, 2011 at 14:23
  • \$\begingroup\$ @akmal Ah I see. This would be the main reason why it is a bad idea to redefine C++ reserved keywords in C code: it confuses programmers who work with both languages :) \$\endgroup\$
    – Lundin
    Sep 28, 2011 at 11:16
  • 1
    \$\begingroup\$ @akmal If you implement something from C++ you better make sure that it works exactly in the same way. In this case you can't achieve it, since there is no way to enforce C to get as strong typing as C++. A conventional "old style" cast would therefore have been better, because then you aren't tricking anyone, including yourself, that you have suddenly achieved strong typing in C. Try to motivate your own statement: why exactly would this C macro be better than "old style"? You can't. Also, it confuses the reader, as we can I see I thought this was C++ myself. \$\endgroup\$
    – Lundin
    Oct 5, 2011 at 11:03

2 Answers 2

4
\$\begingroup\$
#define CONVERT(T, F, v) T val = *(static_cast(T*, src)); \
    F( IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + v ); \
    bw->curPos += length;

    [...]

    case BW_DATA_TYPE_UINT8:
    {
        CONVERT(Uint8_T, Uint8ToLittleEndianAr, 0)
        break;
    }

    case BW_DATA_TYPE_UINT16_LE:
    {
        CONVERT(Uint16_T, Uint16ToLittleEndianAr, 1)
        break;
    }

    case BW_DATA_TYPE_UINT32_LE:
    {
        CONVERT(Uint32_T, Uint32ToLittleEndianAr, 3)
        break;
    }

    case BW_DATA_TYPE_UINT64_LE:
    {
        CONVERT(Uint64_T, Uint64ToLittleEndianAr, 7)
        break;
    }

    case BW_DATA_TYPE_UINT16_BE:
    {
        CONVERT(Uint16_T, Uint16ToBigEndianAr, 1)
        break;
    }

(untested!)
You can go beyond that and even generate the whole case, but this version is a compromise to keep readability.
[EDIT] I missed the bottom of your code, but I suppose you can complete yourself... :-)

\$\endgroup\$
3
  • 8
    \$\begingroup\$ Function-like macros are usually a very bad idea. I think this made the code less readable. \$\endgroup\$
    – Lundin
    Sep 22, 2011 at 14:16
  • \$\begingroup\$ @Lundin, I'm not sure its less readable, but an inline function would be better. \$\endgroup\$ Sep 22, 2011 at 21:10
  • \$\begingroup\$ @Winston Ewert: I would go with less readable. Not strictly because it is in the source but because you can't read it from the debugger it is totally opaque from that point of view. Not to mention all the other problems associated with using function macros. \$\endgroup\$ Sep 23, 2011 at 0:28
2
\$\begingroup\$

First of all, I don't think there is anything wrong with the original code. It is clear what it does and the compiler is competent enough to optimize things that are repeated in every case statement.

But if you really must distill this switch into something that only contains the differences between the case statements you can do like this:

// typedef a function pointer, change "type" with the appropriate types for the function.
typedef void(*ToLittleEndianType)(type in, type out_from, type out_to);

Uint8_T val;
Uint8_T offset;
ToLittleEndianType ToLittleEndianAr;


case BW_DATA_TYPE_UINT8:
{
    val = *(static_cast(Uint8_T*, src));
    offset = 0;
    ToLittleEndianAr = Uint8ToLittleEndianAr;
    break;
}

case BW_DATA_TYPE_UINT16_LE:
{
    val = *(static_cast(Uint16_T*, src));
    offset = 1;
    ToLittleEndianAr = Uint16ToLittleEndianAr;
    break;
}

case BW_DATA_TYPE_UINT32_LE:
{
    val = *(static_cast(Uint32_T*, src));
    offset = 3;
    ToLittleEndianAr = Uint32ToLittleEndianAr;
    break;
}

case BW_DATA_TYPE_UINT64_LE:
{
    val = *(static_cast(Uint64_T*, src));
    offset = 7;
    ToLittleEndianAr = Uint64ToLittleEndianAr;
    break;
}


case BW_DATA_TYPE_UINT16_BE:
{
    val = *(static_cast(Uint16_T*, src));
    offset = 1;
    ToLittleEndianAr = Uint16ToBigEndianAr;
    break;
}

case BW_DATA_TYPE_UINT32_BE:
{
    val = *(static_cast(Uint32_T*, src));
    offset = 3;
    ToLittleEndianAr = Uint32ToBigEndianAr;
    break;
}

case BW_DATA_TYPE_UINT64_BE:
{
    val = *(static_cast(Uint64_T*, src));
    offset = 7;
    ToLittleEndianAr = Uint64ToBigEndianAr;
    break;
}

ToLittleEndianAr(IN val, OUT_FROM &bw->buffer[bw->curPos], OUT_TO &bw->buffer[bw->curPos] + offset);
bw->curPos += length;

NOTE: This code is type safe, unlike macros.

\$\endgroup\$
2
  • \$\begingroup\$ Will this even work? type in will be different for each function. \$\endgroup\$
    – ronag
    Sep 23, 2011 at 10:46
  • \$\begingroup\$ I have no idea what the code is supposed to do, so that's difficult to say. Function pointers can only be used if the functions have the very same parameters. Which in turn means that the functions may or may not need void pointers. It is a C solution, in C++ you could perhaps consider using templates or "functors" instead. \$\endgroup\$
    – Lundin
    Sep 23, 2011 at 14:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.