5
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ I'm assuming a vector is out of the question? \$\endgroup\$ – Jamal Aug 31 '13 at 21:57
  • \$\begingroup\$ well the book haven't mentioned it before, so I think I should use vectors in this exercise :D but pointers haven't been mentioned too... \$\endgroup\$ – mitya221 Aug 31 '13 at 22:13
  • 1
    \$\begingroup\$ Unless you were in a situation where you must use a certain method (such as a school assignment), I'd go for the best one possible. Although, there's nothing wrong with learning the basics (raw pointers). Of course, you can always maintain multiple versions of the same exercise. \$\endgroup\$ – Jamal Aug 31 '13 at 22:16
  • \$\begingroup\$ Would you like to see a version I've come up with using vectors? \$\endgroup\$ – Jamal Aug 31 '13 at 22:32
  • 1
    \$\begingroup\$ If you're getting the correct output, then I'd say it'll work. Nothing wrong with seeing another method. \$\endgroup\$ – Jamal Aug 31 '13 at 22:43
6
\$\begingroup\$

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;
}
\$\endgroup\$
  • \$\begingroup\$ After running your first block through Ideone, the exception is already thrown. To fix this, change the || to && in your function. Also, the loop in main() is printing an extra value. To fix that, change the 8 to 7. In case you haven't ran the OP's code, the expected output is 1 9 12 3 1 0 6 11 (I've also calculated that myself for confirmation). \$\endgroup\$ – Jamal Sep 1 '13 at 4:12
  • \$\begingroup\$ This was a "typo" on my part. I originally had it as a 7 and changed it to an 8 to test that the exception was thrown correctly. I forgot to change it back to a 7. Changing the logical OR to an AND is bad idea though. The condition would NEVER be met. \$\endgroup\$ – Drew Chapin Sep 1 '13 at 4:31
  • \$\begingroup\$ Ah, I see. You're right about the conditions, though. Was being a little hasty trying to check it. \$\endgroup\$ – Jamal Sep 1 '13 at 4:34
  • \$\begingroup\$ Also, once you correct my typo. The output is 1 9 12 3 1 0 6 11, the same as the OP's code. But the second solution is backwards though. \$\endgroup\$ – Drew Chapin Sep 1 '13 at 4:35
  • \$\begingroup\$ Thanks for mentioning Ideone.com too. I had never heard of it before now. Looks handy for when you don't have a compiler to test with :) \$\endgroup\$ – Drew Chapin Sep 1 '13 at 4:37
5
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ +1 That static could also easily kick RVO straight in the teeth. Use a local. \$\endgroup\$ – WhozCraig Sep 1 '13 at 9:04
3
\$\begingroup\$

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, " "));
}
\$\endgroup\$
  • \$\begingroup\$ The solution in this answer has received reviews here. \$\endgroup\$ – Jamal Jun 18 '14 at 18:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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