17
\$\begingroup\$

I am wondering if there is any way to meaningfully shorten the chain of conditionals used here to compare two versions. struct VERSIONCODE represents a version number of the form major.minor.revision and CompareVersions returns EQUAL if they're equal; LHS_NEWER if vLHS is newer, and RHS_NEWER if vRHS is newer.

typedef enum
{
    EQUAL = 0,
    LHS_NEWER,
    RHS_NEWER
} ECOMPARISON;

// Specifically not named "VERSION" to avoid conflicting with common names from third-party libraries etc.
typedef struct
{
    int nMajor;
    int nMinor;
    int nRev;
} VERSIONCODE;

ECOMPARISON CompareVersions(VERSIONCODE vLHS, VERSIONCODE vRHS)
{
    if (vLHS.nMajor > vRHS.nMajor)
    {
        return LHS_NEWER;
    }
    else if (vLHS.nMajor < vRHS.nMajor)
    {
        return RHS_NEWER;
    }
    else// if (vLHS.nMajor == vRHS.nMajor)
    {
        if (vLHS.nMinor > vRHS.nMinor)
        {
            return LHS_NEWER;
        }
        else if (vLHS.nMinor < vRHS.nMinor)
        {
            return RHS_NEWER;
        }
        else// if (vLHS.nMinor == vRHS.nMinor)
        {
            if (vLHS.nRev > vRHS.nRev)
            {
                return LHS_NEWER;
            }
            else if (vLHS.nRev < vRHS.nRev)
            {
                return RHS_NEWER;
            }
            else// if(vLHS.nRev == vRHS.nRev)
            {
                return EQUAL;
            }
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I think it would be good to explicitly define LHS_NEWER=-1 and RHS_NEWER=1, because that's what functions like qsort() expect \$\endgroup\$ – Monty Harder Feb 26 at 22:01
  • \$\begingroup\$ @MontyHarder Yup, thought of that after posting. :) \$\endgroup\$ – Govind Parmar Feb 27 at 0:21
14
\$\begingroup\$

The CompareVersions() function in this answer uses subtraction for comparison.

This is considered to be bad practice - it leads to bugs and potential security holes.

(Yes, the post does say "if we can ensure that the version values are small enough to avoid integer overflow", but that pretty much requires the caller of this function to know the result ahead of time.)

To actually answer the question, I would remove the unnecessary elses:

ECOMPARISON CompareVersions(VERSIONCODE vLHS, VERSIONCODE vRHS)
{
    if (vLHS.nMajor > vRHS.nMajor) return LHS_NEWER;
    if (vLHS.nMajor < vRHS.nMajor) return RHS_NEWER;

    // vLHS.nMajor == vRHS.nMajor

    if (vLHS.nMinor > vRHS.nMinor) return LHS_NEWER;
    if (vLHS.nMinor < vRHS.nMinor) return RHS_NEWER;

    // vLHS.nMinor == vRHS.nMinor

    if (vLHS.nRev > vRHS.nRev) return LHS_NEWER;
    if (vLHS.nRev < vRHS.nRev) return RHS_NEWER;

    return EQUAL;
}

This is much easier to read, and can be seen to be correct by inspection.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ "but that pretty much requires the caller of this function to know the result ahead of time" of course they do. We are handling version numbers. And that answer also shows an alternative. \$\endgroup\$ – L. F. Feb 26 at 4:49
  • \$\begingroup\$ I like that you leave in the comments to tell the next programmer to look at this that, having passed the previous pair of comparisons, we know the more-significant version components are equal. \$\endgroup\$ – Monty Harder Feb 26 at 21:49
21
\$\begingroup\$
typedef enum
{
    EQUAL = 0,
    LHS_NEWER,
    RHS_NEWER
} ECOMPARISON;

A common convention for three-way comparisons is that of strcmp() - return any negative value if the first argument is less than the second, zero if equal, and positive if greater.

If we take that approach, and if we can ensure that the version values are small enough to avoid integer overflow, we can simplify the comparison to just:

int CompareVersions(VERSIONCODE a, VERSIONCODE b)
{
    int diff = a.nMajor - b.nMajor;
    if (diff) return diff;
    diff = a.nMinor - b.nMinor;
    if (diff) return diff;
    return a.nRev - b.nRev;
}

If we can't ensure overflow won't happen, or if we absolutely must return fixed values, we'll need to convert those subtractions into calls to a custom comparison function - perhaps like this:

int compare_int(int a, int b)
{
    /* standard idiom to return -1, 0 or +1 */
    return (a > b) - (b > a);
}

The naming can be improved. Most C conventions use ALL_UPPERCASE only for macros, to call attention to their difference from C code. And most authorities discourage prefixes like the n and v used at the start of variable and member names - I encourage you to research "Hungarian notation" and understand the arguments.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Oops, you're right - fixed. Thanks for pointing that out. \$\endgroup\$ – Toby Speight Feb 24 at 20:17
8
\$\begingroup\$

You can remove the "else". You use conditions that return, so the else isn't needed as there is no other code that can be run.

That'll reduce nesting :

if (vLHS.nMajor > vRHS.nMajor)
{
    return LHS_NEWER;
}
else if (vLHS.nMajor < vRHS.nMajor)
{
    return RHS_NEWER;
}

if (vLHS.nMinor > vRHS.nMinor)
{
    return LHS_NEWER;
}
else if (vLHS.nMinor < vRHS.nMinor)
{
    return RHS_NEWER;
}

if (vLHS.nRev > vRHS.nRev)
{
    return LHS_NEWER;
}
else if (vLHS.nRev < vRHS.nRev)
{
    return RHS_NEWER;
}

return EQUAL;

That's more readable isn't it ? Next, you could wrap the repeated code in a function, which would reduce those 3 conditions to 1 line each, and make it easier to read.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I would say that conditional expressions could be used. \$\endgroup\$ – S.S. Anne Feb 24 at 14:27
  • 2
    \$\begingroup\$ You could remove the remaining else as well to make the code even shorter and more uniform. \$\endgroup\$ – Roland Illig Feb 24 at 16:30
  • \$\begingroup\$ One of those conditionals can only be reduced to one line by putting them in a function if they return always. But since your conditions are if return else if return else continue this cannot ever be captured in a single line \$\endgroup\$ – Cruncher Feb 24 at 17:16
6
\$\begingroup\$

Late to the party, yet another candidate simplification.

Look for inequality starting with most important. The logic flows fairly directly. Also, at most 4 compares.

ECOMPARISON CompareVersions(VERSIONCODE vLHS, VERSIONCODE vRHS) {
  if (vLHS.nMajor != vRHS.nMajor) {
    return (vLHS.nMajor > vRHS.nMajor) ? LHS_NEWER : RHS_NEWER;
  }
  if (vLHS.Minor != vRHS.nMinor) {
    return (vLHS.nMinor > vRHS.nMinor) ? LHS_NEWER : RHS_NEWER;
  }
  if (vLHS.nRev != vRHS.nRev) {
    return (vLHS.nRev > vRHS.nRev) ? LHS_NEWER : RHS_NEWER;
  }
  return EQUAL;
}

Note that without subtraction, no risk of int overflow.

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$
int ultimateLVersion = 1000000 * vLHS.nMajor + 1000 * vLHS.nMinor + 1 * vLHS.nRev;
int ultimateRVersion = 1000000 * vRHS.nMajor + 1000 * vRHS.nMinor + 1 * vRHS.nRev;

if (ultimateLVersion > ultimateRVersion){
  return vLHS;
} else if (ultimateRVersion > ultimateLVersion){
  return vRHS;
} else {
  return EQUAL;
}

Increase the number of zeros if needed.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I'm not sure if I love this or if I hate this. \$\endgroup\$ – Oskar Skog Feb 25 at 14:45
  • 4
    \$\begingroup\$ I do like this approach a lot, and have used it myself. It is especially advantageous in cases where performance matters, because it has the potential to reduce a lot of unpredictable branches. But it does suffer from some risk of overflow. To help catch those problems, I'd want to add some some assertions (at least runtime checks, but preferably compile-time checks for overflow). \$\endgroup\$ – Cody Gray Feb 25 at 16:34
  • 1
    \$\begingroup\$ @CodyGrayIn addition to overflow concerns (which is usually manageable), should .nMajor, or other members have negative values, then pedantically we have more order issues as .nMinor could "beat" .nMajor. Here I'd go for unsigned math and unsigned members and asserts to mitigate that. \$\endgroup\$ – chux - Reinstate Monica Feb 25 at 16:54
  • 1
    \$\begingroup\$ Yup, version numbers should never be negative, @chux. I wouldn't necessary use unsigned types, as that can mask errors via the magic of implicit conversion. Using a signed type as a parameter allows you to actually catch the error with an assert. \$\endgroup\$ – Cody Gray Feb 27 at 2:33

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.