3
\$\begingroup\$

You give this function a pointer and how many bytes you want to extract as a decimal and it returns it.

long long
byte_extract
(void * source, size_t bytes)
{
    long long num = 0, m = 1;
    size_t i;

    for(i = 0; i != bytes; i++)
    {
        num += (((unsigned char*)source)[i]*m);
        m<<=0x8;
    }
    return num;
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ It's rather unclear what you are asking \$\endgroup\$
    – jsanc623
    Nov 7, 2014 at 22:26

4 Answers 4

5
\$\begingroup\$

It does the job, so that's good. It's quite clear and I don't see fundamental flaws in it. There are a few minor things I'd recommend to change:

  • The most important expression in the middle is a bit hard to read. I find it much easier this way: num += m * ((unsigned char*) source)[i]; (Redundant parens removed, multiplication operands switched, minor spacing adjustments)

  • Instead of m<<=0x8; I would write m <<= 8; with more spacing and 0x8 is the same as simply 8

  • The name byte_extract doesn't convey well what you described: "You give this function a pointer and how many bytes you want to extract as a decimal and it returns it." extract_num or extract_bytes_as_num would be better

  • Renaming m to multiplier could be a good idea too

\$\endgroup\$
0
4
\$\begingroup\$

byte_extract is an odd name for a function that returns some kind of integer. A comment explaining its purpose wouldn't hurt.

long long was introduced with C99, so your code should be up to C99 standards. That means that the source should be const. i should be scoped to the loop.

To declutter the most complex line of code, I would perform the cast once, at the beginning of the function. Personally, I would choose to use the |= operator rather than +=, to emphasize that it can be thought of as a bitwise operation. Conversely, when shifting m by 8 bits, there is no advantage to writing that as 0x8.

/* Interprets _bytes_ number of bytes of _source_ as a little-endian integer. */
long long
read_integer
(const void *source, size_t bytes)
{
    const unsigned char *data = (const unsigned char *)source;
    long long num = 0, m = 1;

    for (size_t i = 0; i < bytes; i++)
    {
        num |= data[i] * m;
        m <<= 8;
    }
    return num;
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ +1 for readability and by the way.. I am doing this on C89. Languages from the last millenia you know.. \$\endgroup\$
    – Edenia
    Nov 8, 2014 at 1:29
  • 1
    \$\begingroup\$ Millenium. Millenia is plural. :) I don't think there were any programming languages in the millenium before 1000-2000. \$\endgroup\$
    – Almo
    Nov 8, 2014 at 3:13
4
\$\begingroup\$
(void * source, size_t bytes)

I might name bytes as byte_size or byte_count. I generally use plurals to denote collections, and I think that including either size or count in the name clarifies the purpose.

for(i = 0; i != bytes; i++)

As 200_success already pointed out, this could be better written as

for ( i = 0; i < bytes; i++ )

The main reason is that using != is fragile. If something happens that changes i for some other reason than currently planned, you can skip past the single loop ending value. Once you do that, it will run until it loops entirely around or crashes. The < restricts the valid values more. Note: this is not a bug in the current program; nor is it likely to impact this function. The issue is that there are other circumstances where this pattern is much riskier.

    m <<= 8;

Assuming that you take the advice in the other answers, I still think that there's something worth noting here. You are making an assumption here about the size of an unsigned char (that it's one byte or eight bits). This assumption is generally true but not always.

One possible fix is to use a uint8_t, but that type may not exist in your C. A second solution is to change the value based on your platform's byte size:

#include <limits.h>
const size_t CHAR_WIDTH = sizeof(unsigned char) * CHAR_BIT;
// skip irrelevant code
    m <<= CHAR_WIDTH;

That should work on any platform regardless of platform-specific byte sizes.

\$\endgroup\$
1
  • \$\begingroup\$ As i stated before, I'm using C89. for ( size_t i = 0; i < bytes; i++ ) Is illegal in C89. \$\endgroup\$
    – Edenia
    Nov 8, 2014 at 2:09
1
\$\begingroup\$
long long byte_extract (void * source, size_t bytes)
{
    long long num = 0, m = 1;
    size_t i;

    for(i = 0; i < bytes; i++)
    {
        num += (((unsigned char*)source)[i]*m);
        m<<=0x8;
    }
    return num;
}

This code is taking first the left most byte, multiply by 1, then second byte multiply by 256, etc. This is counter intuitive, as it assumes that

  1. The bytes are already in the range 0x00 through 0x09 and
  2. The least significant byte is first.
  3. Each byte is 8 bits

The question does not indicate the actual format of the incoming bytes. If they are already in binary format, then fine, however if they are in char format, then each byte needs to be converted to binary. I.E. byte - 0x30. (assuming ascii format).

Also the parameter bytes must be <= sizeof(long long int).

\$\endgroup\$
2
  • \$\begingroup\$ Yes, this is improved already. \$\endgroup\$
    – Edenia
    Nov 8, 2014 at 14:06
  • \$\begingroup\$ What is your recommendation? Collapsing the signature into one line and changing != into <? \$\endgroup\$ Nov 9, 2014 at 9:25

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.