Noissue - Return value of filter match expression directly

lib/filterClasses.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 701 matching lines @@
    * @param {string} location URL to be tested
    * @param {number} typeMask bitmask of content / request types to match
    * @param {string} [docDomain] domain name of the document that loads the URL
    * @param {boolean} [thirdParty] should be true if the URL is a third-party
    *                               request
    * @param {string} [sitekey] public key provided by the document
    * @return {boolean} true in case of a match
    */
   matches(location, typeMask, docDomain, thirdParty, sitekey)
   {
-    if (this.contentType & typeMask &&
-        (this.thirdParty == null || this.thirdParty == thirdParty) &&
-        this.isActiveOnDomain(docDomain, sitekey) && this.regexp.test(location))
-    {
-      return true;
-    }
-    return false;
+    return (this.contentType & typeMask) != 0 &&

[Manish Jethani, 2018/09/04 13:46:06]

The first condition here needs to evaluate to a boolean value, just so the
function returns false rather than 0 when the check fails.

[Jon Sonesen, 2018/09/05 14:16:38]

Yeah, I see what you did there. Stylistically I prefer this as well, it is
easier to read. I think it is not crazy hard to read the masking, as most
programmers are aware of masks but this just looks nicer and is more uniform
with how most programmers do things (my first choice would have been to return a
bool directly with the mask as you have done here had I implemented this)

Thanks!

LGTM

+           (this.thirdParty == null || this.thirdParty == thirdParty) &&
+           this.isActiveOnDomain(docDomain, sitekey) &&
+           this.regexp.test(location);
   }
 });
 
 /**
  * Creates a RegExp filter from its text representation
  * @param {string} text   same as in Filter()
  * @return {Filter}
  */
 RegExpFilter.fromText = function(text)
 {
@@ skipping 442 matching lines @@
 
   /**
    * Script that should be executed
    * @type {string}
    */
   get script()
   {
     return this.body;
   }
 });