Compare version numbers

Question

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;
            }
        }
    }
}

Answer 1

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.

Answer 2

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.

Answer 3

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.

Answer 4

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.

Answer 5

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.