5
\$\begingroup\$

Please review and let me know which version is better. I'm just trying to find any caveats or better ways to accomplish the a version comparison independent from Android and iOS.

Formats accepted: '1.0.0' or '1' formats and an operator, e.g: compareVersion('1', '1.2.0', '>')

export function compareVersion(version1, version2, operator) {
        const formattedV1 = version1.split(".");
        const formattedV2 = version2.split(".");

        let diff = 0;

        if (formattedV1.length !== formattedV2.length) {
            const lengthDiff = formattedV1.length - formattedV2.length;
            for (let index = 0; index < Math.abs(lengthDiff); index += 1) {
                if (lengthDiff > 0) {
                    formattedV2.push("0");
                } else {
                    formattedV1.push("0");
                }
            }
        }

        for (let index = 0; index < formattedV1.length; index += 1) {
            if (diff === 0) {
                const v1 = parseInt(formattedV1[index]);
                const v2 = parseInt(formattedV2[index]);
                if (isNaN(v1) || isNaN(v2)) {
                    throw new Error("Problem comparing versions: not a valid number");
                }
                if (v1 < v2) {
                    diff = -1;
                }
                if (v1 > v2) {
                    diff = 1;
                }
            }
        }

        switch (operator) {
        case "=":
        case "==":
            return diff === 0;
        case ">=":
            return diff >= 0;
        case "<=":
            return diff <= 0;
        case ">":
            return diff > 0;
        case "<":
            return diff < 0;

        default:
            throw new Error("Problem comparing versions");
        }
    }

OR

export function compareVersions(a, b, operator) {
  const aParts = a.split('.');
  const bParts = b.split('.');

  const pairs = [];

  for (let index = 0; index < Math.max(aParts.length, bParts.length); index += 1) {
    pairs.push({
      a: parseInt(aParts[index]),
      b: parseInt(bParts[index]),
    });
  }

  let diff = 0;

  pairs.forEach((pair) => {
    if (diff === 0) {
      if (pair.a > pair.b) {
        diff = 1;
      }
      if (pair.b > pair.a) {
        diff = -1;
      }
      if (!isNaN(pair.a) && isNaN(pair.b)) {
        diff = 1;
      }
      if (isNaN(pair.a) && !isNaN(pair.b)) {
        diff = -1;
      }
    }
  });

  switch (operator) {
    case '=':
    case '==':
      return diff === 0;
    case '>=':
      return diff >= 0;
    case '<=':
      return diff <= 0;
    case '>':
      return diff > 0;
    case '<':
      return diff < 0;

    default:
      throw new Error('Problem comparing versions');
  }
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

One role

Be careful to do only what the functions should.

The function is called compareVersions however what it does is validate and compare versions.

The potential throw is the result of validation and has nothing to do with comparing the values.

As you have not called the function validateAndCompareVersions you have over stepped its role.

How to throw

If (and try to avoid throwing exceptions), if you must throw, throw correctly.

In this case the first throw should be a RangeError and the message should make sense.

You have very poor error message "Problem comparing versions: not a valid number"; The first part is redundant, exception contains a trace that will locate the "problem" and the "Problem" is implied in the fact that this is an exception.

"not a valid number" The arguments are version strings, "1.0.0" is not a number. The error should indicate the problem explicitly.

 throw new RangeError("Invalid version string: '" + (isNaN(v1) ? v1 : v2) + "'");

The second exception is a little better, but could be improved

 throw new RangeError("Invalid operator: " + o);

How to catch

Only catch what is thrown for you. Higher level catchers will handle the rest.

Having the throws in the function means you must add additional support code outside the function. Try catches break the flow of the code, not having a try catch means that a input error will halt the app.

try {
    compareVersions(a,b,type);
} catch(e) {
    if (e.name === "RangeError") {
        // do what is needed to prevent app from stopping
    } else {
        throw e; // rethrow for development cycle or high level catchers
    }
}

You may need to be a little more specific, you can either extend an existing error or define a new error. Extending is the JS way so using the name property

// in function
const error = new RangeError("Invalid version string: '" + (isNaN(v1) ? v1 : v2) + "'");
error.name = "InvalidVersionStr";
throw error;

// outside function
try {
    compareVersions(a,b,type);
} catch(e) {
    if (e.name === "InvalidVersionStr") {
        // do what is needed to prevent app from stopping
    } else {
        throw e; // rethrow for development cycle or high level catchers
    }
}

Validate or normalize

It is best to avoid exceptions.

In JavaScript we can pretend we have a 3 state Boolean, true, false, and undefined. We can use the 3rd state to signal an error without needing to break flow or halt execution. You can return undefined with return;

Then you calling function need only handle the undefined

 const versionResult = compareVersions(a, b, type);
 if (versionResult === undefined) { /* put the spanner here */ }

Better yet the function should assume all is good and just return true or false.

Validate the version strings at the source and deal with it when you get it (that is where the problem actually is)

You can either validate the version string or normalize the string

function normaliseVersionStr(ver) {
    return /^\d*\.\d*\.\d*$|^\d*$/.test(ver) ? ver : "0";
}
function validateVersionStr(ver) {  // returns false if string not valid, else true
    return /^\d*\.\d*\.\d*$|^\d*$/.test(ver);
}

// Good strings returned as they are
normaliseVersionStr("1.0.0")
normaliseVersionStr("1")
normaliseVersionStr("1.000.0")

// Bad string returned as version 0
normaliseVersionStr("1.0.0A")
normaliseVersionStr("1.")
normaliseVersionStr("1.0")
normaliseVersionStr("")

Alternative solution

Now that you can trust the arguments you get you can write a better function as you dont need to bother with all the possible edge cases.

There are many ways to do this, and they depend on what you define as same, greater, less. I will assume the following

 01 == 1 == 1.0.0 == 01.0.0 == 1.00.00
 0.01.0 == 0.1.0
 0.0.1 < 0.1.0
 2.0.1 < 2.1.0
 1.9.0 < 2

If we then pad the strings to match (each version str) parts sizes, and remove leading zeros.

 1 and 1.0.0 become 100 and 100
 1 and 2 become 1 and 2
 1 and 0.0.1 become 100 and 1
 1.99.0 and 1 become 1990 and 1000

Then use eval to do the final operation. If you don't like eval, you can use new Function

Example

It is assumed that the operator is defined in the source (not as an user input). If the operator is a user input string then you should validate or normalize that string before calling the function.

An invalid operator will throw an exception.

// logical operator optional. Default "=="
// "==", "<", ">", "<=", ">="
// "=" is considered to be "=="
// can also use "!=", "!==", "==="
// strA, strB must be valid version strings
function compareVersionStrings(strA, strB, operator = "==") {    
    const a = strA.split("."), b = strB.split(".");
    const len = Math.max(a.length, b.length);
    var valA = a.shift(), valB = b.shift(), i = 1;
    while (i < len) {
        const vA = a[i] !== undefined ? a[i] : "";
        const vB = b[i] !== undefined ? b[i] : "";
        const digits = Math.max(vA.length, vB.length);
        valA += vA.padStart(digits, "0");
        valB += vB.padStart(digits, "0");
        i++;
    }
    valA = valA.replace(/0*\d$/,"");
    valB = valB.replace(/0*\d$/,"");
    operator = operator === "=" ? "==" : operator;
    return eval(`${valA} ${operator} ${valB}`);
    // or
    return (new Function(`return ${valA} ${operator} ${valB})`)();
}
\$\endgroup\$

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.