Issue 5728 - Replace Services.vc.compare

lib/notification.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 49 matching lines @@
     return translations[locale];
 
   let languagePart = locale.substring(0, locale.indexOf("-"));
   if (languagePart && languagePart in translations)
     return translations[languagePart];
 
   let defaultLocale = "en-US";
   return translations[defaultLocale];
 }
 
+function parseVersionComponent(comp)
+{
+  if (comp == "*")
+    return Infinity;
+  return parseInt(comp, 10) || 0;
+}
+
 function compareVersion(v1, v2)
 {
   let regexp = /^(.*?)([a-z].*)?$/i;

[Manish Jethani, 2017/09/22 23:28:51]

If "57.0a9" vs "57.0a10" matters, we could handle it with a little extra effort:

  const re = /([\d*]+\.?[\d*]*)?([a-zA-Z]+)?(\d+)?/;

  let [, numA1, strB1, numC1, strD1] = re.exec(v1);
  let [, numA2, strB2, numC2, strD2] = re.exec(v2);

Then we can simply repeat the number and string comparisons for the C and D
components respectively.

[Sebastian Noack, 2017/09/23 00:32:50]

This would add notable complexity to this implementation, this regular
expression alone is rather complex, and incorrect (the 4th group is missing).

   let [, head1, tail1] = regexp.exec(v1);
   let [, head2, tail2] = regexp.exec(v2);
   let components1 = head1.split(".");
   let components2 = head2.split(".");
 
   for (let i = 0; i < components1.length ||
                   i < components2.length; i++)
   {
-    let comp1 = components1[i];
-    let comp2 = components2[i];
-
-    let result = (comp1 == "*" ? Infinity : parseInt(comp1, 10) || 0) -
-                 (comp2 == "*" ? Infinity : parseInt(comp2, 10) || 0) || 0;

[Wladimir Palant, 2017/09/22 10:11:49]

The readability of this statement is very suboptimal. In particular, I didn't
immediately realize that "1.*" and "1.*" will compare correctly (`|| 0` converts
NaN to zero). I suggest adding an inner function to parse components:

  function parseComponent(comp)
  {
    if (comp == "*")
      return Infinity;
    return parseInt(comp, 10) || 0;
  }

Then you can change this into:

  let result = (parseComponent(components1[i]) - parseComponent(components2[i]))
|| 0;

[Sebastian Noack, 2017/09/23 00:32:50]

I kinda liked the one-liner, but what I like about your suggestion is that we
don't need those two temporary variables. Done.

+    let result = parseVersionComponent(components1[i]) -
+                 parseVersionComponent(components2[i]) || 0;
 
     if (result != 0)
       return result;
   }
 
-  return tail1 == tail2 ? 0 : !tail1 || tail2 && tail1 > tail2 ? 1 : -1;

[Wladimir Palant, 2017/09/22 10:11:49]

This part isn't covered by unit tests (so far these were assuming that version
comparison is tested elsewhere), so you need to add some. At the very least, you
need to verify that "27.0b1" < "27.0" and "27.0" < "27.1a1".

Also, this might be valid for compilers, but this statement is very hard to
read. I suggest that you change at least one conditional into a proper if
statement (better both) and add parentheses:

  if (tail1 == tail2)
    return 0;
  if (!tail1 || (tail2 && tail1 > tail2))
    return 1;
  return -1;

For reference, this code won't compare "57.0a9" and "57.0a10" correctly. We can
live with that I think, but you should add a comment noting that.

[Manish Jethani, 2017/09/22 23:23:41]

FWIW "tail2 && tail1 > tail2" can be just "tail1 > tail2" as tail2 will never be
an empty string, just undefined, in which case the comparison will evaluate to
false anyway.

[Sebastian Noack, 2017/09/23 00:32:50]

Done.

[Sebastian Noack, 2017/09/23 00:32:50]

This is actually true (it wasn't in earlier when I extracted the tails
differently). But I have my doubts that Wladimir would approve a simplification,
relying on the fact that any compassion with undefined returns false. ;)

FWIW, I'm not sure either, how great an idea that is. If we'd ever touch this
code again, and some change causes tailN to be an empty string instead again, it
wouldn't be very obvious what implication this has on the logic here.

+  // Compare version suffix (e.g. 0.1alpha < 0.1b1 < 01.b2 < 0.1).
+  // However, note that this is a simple string comparision, meaning: b10 < b2
+  if (tail1 == tail2)
+    return 0;
+  if (!tail1 || tail2 && tail1 > tail2)
+    return 1;
+  return -1;
 }
 
 /**
  * The object providing actual downloading functionality.
  * @type {Downloader}
  */
 let downloader = null;
 let localData = [];
 
 /**
@@ skipping 401 matching lines @@
     else if (index != -1 && forceValue !== true)
       categories.splice(index, 1);
 
     // HACK: JSON values aren't saved unless they are assigned a
     // different object.
     Prefs.notifications_ignoredcategories =
       JSON.parse(JSON.stringify(categories));
   }
 };
 Notification.init();