Compare version numbers

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,
} 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)
{
}
else if (vLHS.nMajor < vRHS.nMajor)
{
}
else// if (vLHS.nMajor == vRHS.nMajor)
{
if (vLHS.nMinor > vRHS.nMinor)
{
}
else if (vLHS.nMinor < vRHS.nMinor)
{
}
else// if (vLHS.nMinor == vRHS.nMinor)
{
if (vLHS.nRev > vRHS.nRev)
{
}
else if (vLHS.nRev < vRHS.nRev)
{
}
else// if(vLHS.nRev == vRHS.nRev)
{
return EQUAL;
}
}
}
}

• I think it would be good to explicitly define LHS_NEWER=-1 and RHS_NEWER=1, because that's what functions like qsort() expect Feb 26 '20 at 22:01
• @MontyHarder Yup, thought of that after posting. :) Feb 27 '20 at 0:21

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.

• 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. Feb 26 '20 at 21:49
typedef enum
{
EQUAL = 0,
} 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.

• Oops, you're right - fixed. Thanks for pointing that out. Feb 24 '20 at 20:17

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)
{
}
else if (vLHS.nMajor < vRHS.nMajor)
{
}

if (vLHS.nMinor > vRHS.nMinor)
{
}
else if (vLHS.nMinor < vRHS.nMinor)
{
}

if (vLHS.nRev > vRHS.nRev)
{
}
else if (vLHS.nRev < vRHS.nRev)
{
}

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.

• I would say that conditional expressions could be used. Feb 24 '20 at 14:27
• You could remove the remaining else as well to make the code even shorter and more uniform. Feb 24 '20 at 16:30
• 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 Feb 24 '20 at 17:16

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) {
}
if (vLHS.Minor != vRHS.nMinor) {
}
if (vLHS.nRev != vRHS.nRev) {
}
return EQUAL;
}


Note that without subtraction, no risk of int overflow.

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.

• I'm not sure if I love this or if I hate this. Feb 25 '20 at 14:45
• 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). Feb 25 '20 at 16:34
• @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. Feb 25 '20 at 16:54
• 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. Feb 27 '20 at 2:33