8
\$\begingroup\$
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>

typedef int STATUS;
#define ERROR -1
#define OKAY 0

struct version
{
    unsigned char major;
    unsigned char minor;
    unsigned char build;
    unsigned char patch;
};
STATUS is_less_than(struct version * original, struct version *compared, bool *result)
{
    if(original == NULL || compared == NULL || result == NULL)
    {
        result = NULL;
        return ERROR;
    }
    *result = false;

    if(original->major < compared->major)
    {
        *result = true;
    }
    else if(original->major == compared->major) // else if the major >= major
        {

            if(original->minor < compared->minor)
            {
                *result = true;
            }
            else if(original->minor == compared->minor)
            {
                if(original->build < compared->build)
                {
                    *result = true;
                }
                else if(original->build == compared->build)
                {
                    if(original->patch < compared->patch)
                    {
                        *result = true;
                    }
                    else if(original->patch == compared->patch)
                    {
                        *result = false;
                    }
                }
            }
        }


    return OKAY;

}

Is there a cleaner way to do this?

\$\endgroup\$
  • 1
    \$\begingroup\$ Code golf version, just for fun \$\endgroup\$ – JollyJoker Apr 17 at 11:09
  • \$\begingroup\$ If the build number is monotonically increasing, you could just compare the build number \$\endgroup\$ – Cole Johnson Apr 23 at 19:13
14
\$\begingroup\$

Yes, there is a cleaner way:

if (a.major != b.major) {
    *result = a.major < b.major;
} else if (a.minor != b.minor) {
    *result = a.minor < b.minor;
} else if (a.patch != b.patch) {
    *result = a.patch < b.patch;
} else {
    *result = a.build < b.build;
}
return OKAY;

I reordered patch to come before build since that's how it is usually done. If your version scheme is different from this, good luck.

Instead of unsigned char I would choose uint32_t so that your code can handle versions like 1.0.20190415. If you need to handle version numbers with millisecond timestamps, each version component would need to be uint64_t.

\$\endgroup\$
  • \$\begingroup\$ Nice catch on the patch, build ordering. \$\endgroup\$ – Costantino Grana Apr 17 at 3:43
  • 2
    \$\begingroup\$ If you really want to handle versions like 1.0.20190415, unsigned int is not portable enough; unsigned long is. \$\endgroup\$ – L. F. Apr 17 at 8:35
  • \$\begingroup\$ Since when is 1.0.20190415 valid? Version resources can't handle > 65535 in any component. \$\endgroup\$ – Joshua Apr 17 at 18:06
  • 3
    \$\begingroup\$ @Joshua what is "version resources"? Maven can, and pkgsrc also. I guess pkg-config can handle the same, and so on. \$\endgroup\$ – Roland Illig Apr 17 at 19:58
  • 2
    \$\begingroup\$ @Joshua I don't see anything in the question about Win32 or .Net. \$\endgroup\$ – Zymus Apr 17 at 21:29
8
\$\begingroup\$

I don't see any advantage to having the function take three pointers (two for input and one for output) and return a status code. As a result of that unnecessarily error-prone design, the function has to handle the possibility of null pointers, and the caller is expected to handle a status code. But why should such a simple comparison have these failure modes at all?

The danger is further complicated by the fact that neither of the in-parameters is declared const.

Just pass the two versions by value, and you would eliminate all of that complication! On any modern 32-bit or 64-bit processor, passing a four-byte struct by value should actually be more efficient than passing it by reference — especially since you don't have to dereference the pointers to access each field.

With all of the potential errors out of the way, taking @RolandIllig's suggestion, you could then reduce it down to one chained conditional expression:

bool is_less_than(struct version a, struct version b) {
    return a.major != b.major ? a.major < b.major :
           a.minor != b.minor ? a.minor < b.minor :
           a.patch != b.patch ? a.patch < b.patch :
                                a.build < b.build;
}

I'd go further and recommend using unsigned short instead of unsigned char for the fields. Using unsigned char for numeric values is awkward, since you would have to cast them when using printf(). On a 64-bit architecture, a struct with four 2-byte fields would occupy 64 bits, so you wouldn't be saving anything by using unsigned char instead of unsigned short. It's also entirely conceivable that you might need mire than 256 builds within a patch level.

\$\endgroup\$
  • \$\begingroup\$ That code really looks beautiful. There's one thing I don't get though. Can you elaborate on the "you would have to cast them" part? To my knowledge, when used in printf("%u", a.major), both unsigned char and unsigned short behave the same. The different behavior would only occur in C++, when doing std::cout << a.major. This is a C question though. \$\endgroup\$ – Roland Illig Apr 24 at 5:51
  • \$\begingroup\$ @RolandIllig You're right: type promotion would happen implicitly with printf("%d", a.major), so it wouldn't be a significant concern. \$\endgroup\$ – 200_success Apr 24 at 5:57
4
\$\begingroup\$

Return status

You create this:

typedef int STATUS;
#define ERROR -1
#define OKAY 0

which is basically a boolean status. Personally, I'd return a straight bool.

Bug/Not what you mean

Doing a

result = NULL;

is changing the local variable (parameter) result. It's not setting the result to NULL. In fact the caller won't probably have a pointer at all, but just a bool, which cannot properly be NULL.

Shorter version

I'm not sure this is cleaner, but here I go:

bool is_less_than(struct version * original, struct version *compared, bool *result)
{
    if(original == NULL || compared == NULL || result == NULL)
        return false;

    *result = original->major < compared->major || original->major == compared->major && (
              original->minor < compared->minor || original->minor == compared->minor && (
              original->build < compared->build || original->build == compared->build && (
              original->patch < compared->patch)));

    return true;
}

Next time, add a driver/test suite to your question, to ease the life of people answering. This can be one:

int main(void) 
{
    struct version ref      = { 1, 2, 21, 8 };
    struct version lower1   = { 0, 2, 21, 8 };
    struct version lower2   = { 1, 1, 21, 8 };
    struct version lower3   = { 1, 2, 20, 8 };
    struct version lower4   = { 1, 2, 21, 7 };
    struct version equal    = { 1, 2, 21, 8 };
    struct version higher1  = { 2, 2, 21, 8 };
    struct version higher2  = { 1, 3, 21, 8 };
    struct version higher3  = { 1, 2, 22, 8 };
    struct version higher4  = { 1, 2, 21, 9 };

#define TEST(a,b,expect1,expect2)\
    do {\
        bool result1, result2;\
        is_less_than((a), (b), &result1);\
        is_less_than((b), (a), &result2);\
        puts(result1==(expect1) && result2==(expect2)?"ok":"failed");\
    } while(0)
#define TESTL(a,b) TEST(a,b,true,false)
#define TESTE(a,b) TEST(a,b,false,false)
#define TESTH(a,b) TEST(a,b,false,true)

    TESTL(&lower1, &ref);
    TESTL(&lower2, &ref);
    TESTL(&lower3, &ref);
    TESTL(&lower4, &ref);
    TESTE(&equal, &ref);
    TESTH(&higher1, &ref);
    TESTH(&higher2, &ref);
    TESTH(&higher3, &ref);
    TESTH(&higher4, &ref);

    return 0;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ As for every comparator function, the driver/test should compare each pair of example data to at least ensure that the ordering is transitive and that less(x, x) is never true. \$\endgroup\$ – Roland Illig Apr 17 at 3:49
  • \$\begingroup\$ @RolandIllig Updated. Thank you for the suggestion. \$\endgroup\$ – Costantino Grana Apr 17 at 4:15
  • \$\begingroup\$ What is a "driver" in this context? \$\endgroup\$ – the_endian Apr 17 at 14:58
  • 1
    \$\begingroup\$ @the_endian it's the program that runs the tests. Probably a really bad word play based on the "test driver" for cars. There's also the related term "compiler driver" for the main program of GCC or Clang that calls the internal subprograms like the preprocessor, the actual compiler, the assembler and the linker. \$\endgroup\$ – Roland Illig Apr 17 at 20:02
  • 1
    \$\begingroup\$ See en.wikipedia.org/wiki/Test_stub. This terminology comes from before unit testing frameworks were common, and now I feel old. \$\endgroup\$ – Kendall Lister Apr 18 at 6:40

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.