Split a long integer into eight 4-bit values

Question

It's an exercise from a book. There is a given long integer that I should convert into 8 4-bit values. The exercise is not very clear for me, but I think I understood the task correctly.

I googled how to return with an array in a function. In the splitter function I add the last 4 bits into the values array, then cut the them from the original number.

Is this a good way to do this?

#include <iostream>

int * splitter(long int number)
{
    static int values[8];

    for (int i = 0; i < 8; i++)
    {
        values[i] = (int)(number & 0xF);
        number = (number >> 4);
    }

    return values;
}

int main()
{
    long int number = 432214123;
    int *values;
    values = splitter(number);
    for (int i = 7; i >= 0; i--)
        std::cout << values[i] << " ";
    return 0;
}

Answer 1

There are only three issues I take with your code...

1. The caller of the splitter function has the possibility of trying to access a value outside of the returned array and could cause a memory access violation if they did so. For this reason alone, it would be better to go with the vector approach that @Jamal posted.
2. You should use the C++ style casting (e.g. static_cast<int>(number & 0XF)) instead of the C style casting (e.g. (int)(number & 0xF)).
3. You should use std::int32_t instead of long or std::uint32_t instead of unsigned long. long is not gauranteed to be 32 bits, especially on 64 bit platforms. I don't take as big of an issue with this as some do though because on most systems, it will be at least 32 bits.

Here is a solution that only returns one nybble at a time and throws an exception if the caller tries to access an invalid portion of the of number. Since your book hasn't talked about pointers or vectors yet, this might actually be the type of approach it is looking for (possibly minus the exception throwing if it hasn't talked about exceptions yet either). If you take away the exception throwing, the caller still cannot cause a memory access violation though. If they passed something lower than 0 or greater than 7 as the value of part they would just get a return value of 0.

#include <iostream>
#include <stdexcept>
#include <cstdint>

unsigned short get_nybble( std::uint32_t number, const unsigned short part )
{
    if( part > 7 )
        throw std::out_of_range("'part' must be a number between 0 and 7");
    return (number >> (4 * part)) & 0xF;
}

int main( int argc, char* argv[] )
{
    std::uint32_t number = 432214123;
    try {
        for( short i = 7; i >= 0; i-- )
            std::cout << get_nybble(number, i) << " ";
    } catch( std::exception& ex ) {
        std::cerr << "Error: " << ex.what() << std::endl;
    }
    return 0;
}

This may not be the best solution, but another approach I find interesting is an anonymous union combined with a bit field struct. This works with the GNU G++ compiler and should also work with Visual C++. Note that as I understand it this is potentially unsafe on non x86 systems due to the structure packing which I'd bet your book has not talked about yet either.

#include <iostream>
#include <cstdint>

#pragma pack(push, 1)
typedef union 
{
    std::uint32_t value;
    struct 
    {
        std::uint32_t part1: 4;
        std::uint32_t part2: 4;
        std::uint32_t part3: 4;
        std::uint32_t part4: 4;
        std::uint32_t part5: 4;
        std::uint32_t part6: 4;
        std::uint32_t part7: 4;
        std::uint32_t part8: 4;
    };
} nybbles;
#pragma pack(pop)

int main( int argc, char* argv[] )
{
    nybbles num;
    num.value = 432214123;
    std::cout << "Parts = "
        << num.part8 << ", "
        << num.part7 << ", "
        << num.part6 << ", "
        << num.part5 << ", "
        << num.part4 << ", "
        << num.part3 << ", "
        << num.part2 << ", "
        << num.part1 << std::endl;
    return 0;
}

Answer 2

The C++ standard doesn't guarantee that a long is 4 bytes — it may be longer. If you want 8 nibbles, then make number an int32_t. If you want to split a long into however many nibbles it takes, then use 2 * sizeof number instead of 8.

Long decimal literals should have an L suffix: long int number = 432214123L;

It's fine to use a static array, but you should clearly document that fact in a comment. Also be aware that returning static values makes the design non-reentrant, i.e. not thread-safe. While you are at it, your comment should also mention that the results are least-significant-nibble first.

Answer 3

This is how I would've implemented it with extensive use of the STL and C++11. It is a bit shorter than yours, but also more idiomatic. I've added comments throughout to help explain what each part does.

The main thing to note here is that you should return a container object instead of a pointer to a static C array. It's best not to mess with raw pointers in C++ when possible, especially when it makes more sense to return a container object itself. The compiler should be able to optimize this via Return Value Optimization (RVO) as well. This object should then not be static.

This does produce the same output as yours:

#include <algorithm> // std::reverse_copy()
#include <cstdint>   // std::int32_t
#include <iostream>
#include <iterator>  // std::ostream_iterator
#include <vector>

// you could use a typedef to save some typing
// also to give more meaning to your vector
typedef std::vector<int> SplitValues;

SplitValues splitter(std::int32_t number)
{
    SplitValues values(8);

    // work on vector elements using iterators
    for (auto& iter : values)
    {
        iter = static_cast<int>(number & 0xF); // cast the C++ way
        number >>= 4; // shorter form
    }

    return values;
}

int main()
{
    std::int32_t number = 432214123;

    // local vector assigned to one returned from function
    SplitValues values = splitter(number);

    // display vector in reverse order using ostream iterator
    // this is done with one (wrapped) line and no loop
    std::reverse_copy(values.begin(), values.end(),
        std::ostream_iterator<int>(std::cout, " "));
}