fix(targeting-map): refactored to extract a TargetingMap class to DRY…

… slot and service
krux · Sep 17, 2016 · b60e34d · b60e34d
1 parent b8b1a1c
commit b60e34d
Show file tree

Hide file tree

Showing 8 changed files with 276 additions and 73 deletions.
diff --git a/README.md b/README.md
@@ -123,7 +123,7 @@ npm run tdd
 
 ## Issue Guidelines
 
-Please either add a failing [unit test](./test/unit) or include a [jsfiddle](http://jsfiddle.net) that distills and reproduces the issue.
+Please add a failing [bug test](./test/bugs) and create a Pull Request.
 
 # License
 

diff --git a/src/PubAdsService.js b/src/PubAdsService.js
@@ -1,5 +1,6 @@
 import Service from './Service';
 import Slot from './Slot';
+import TargetingMap from './TargetingMap';
 
 /**
  * Publisher Ads service. This service is used to fetch and show ads from your
@@ -14,7 +15,7 @@ export default class PubAdsService extends Service {
   constructor(gt) {
     super(gt, PubAdsService._name);
     this._categoryExclusions = [];
-    this._targeting = {};
+    this._targeting = new TargetingMap();
     this._options = {
       collapseEmptyDivs: false,
       collapseBeforeAdFetch: false,
@@ -161,7 +162,9 @@ export default class PubAdsService extends Service {
    * request ads using {@link PubAdsService#refresh}.
    */
   disableInitialLoad() {
-    this._options.initialLoad = false;
+    this._unlessEnabled(() => {
+      this._options.initialLoad = false;
+    });
   }
 
   /**
@@ -175,12 +178,9 @@ export default class PubAdsService extends Service {
    * was called after the service was enabled.
    */
   enableAsyncRendering() {
-    if (this._enabled) {
-      return false;
-    } else {
+    return this._unlessEnabled(() => {
       this._options.asyncRendering = true;
-      return true;
-    }
+    });
   }
 
   /**
@@ -194,12 +194,9 @@ export default class PubAdsService extends Service {
    * called after the service was enabled.
    */
   enableSingleRequest() {
-    if (this._enabled) {
-      return false;
-    } else {
+    return this._unlessEnabled(() => {
       this._options.singleRequest = true;
-      return true;
-    }
+    });
   }
 
   /**
@@ -212,12 +209,9 @@ export default class PubAdsService extends Service {
    * called after the service was enabled.
    */
   enableSyncRendering() {
-    if (this._enabled) {
-      return false;
-    } else {
+    return this._unlessEnabled(() => {
       this._options.syncRendering = true;
-      return true;
-    }
+    });
   }
 
   /**
@@ -240,11 +234,7 @@ export default class PubAdsService extends Service {
    * @returns {PubAdsService} The service object on which the method was called.
    */
   setTargeting(key, value) {
-    if (Array.isArray(value)) {
-      this._targeting[key] = value;
-    } else {
-      this._targeting[key] = [value];
-    }
+    this._targeting.set(key, value);
     return this;
   }
 
@@ -256,7 +246,7 @@ export default class PubAdsService extends Service {
    * array if there is no such key.
    */
   getTargeting(key) {
-    return this._targeting[key] || [];
+    return this._targeting.get(key);
   }
 
   /**
@@ -265,7 +255,7 @@ export default class PubAdsService extends Service {
    * @returns {!Array<string>} Array of targeting keys. Ordering is undefined.
    */
   getTargetingKeys() {
-    return Object.keys(this._targeting);
+    return this._targeting.keys();
   }
 
   /**
@@ -275,7 +265,7 @@ export default class PubAdsService extends Service {
    * @returns {PubAdsService} The service object on which the method was called.
    */
   clearTargeting(key) {
-    delete this._targeting[key];
+    this._targeting.clear(key);
     return this;
   }
 
@@ -292,13 +282,10 @@ export default class PubAdsService extends Service {
    * after the service was enabled.
    */
   collapseEmptyDivs(optCollapseBeforeAdFetch = false) {
-    if (this._enabled) {
-      return false;
-    } else {
+    return this._unlessEnabled(() => {
       this._options.collapseEmptyDivs = true;
       this._options.collapseBeforeAdFetch = optCollapseBeforeAdFetch;
-      return true;
-    }
+    });
   }
 
   /**
@@ -330,9 +317,9 @@ export default class PubAdsService extends Service {
    * @param {boolean} centerAds true to center ads, false to left-align them.
    */
   setCentering(centerAds) {
-    if (!this._enabled) {
+    this._unlessEnabled(() => {
       this._options.centerAds = centerAds;
-    }
+    });
   }
 
   /**
@@ -462,4 +449,20 @@ export default class PubAdsService extends Service {
     return this;
   }
 
+  /**
+   * Conditionally execute a function unless the service has already been
+   * enabled.
+   *
+   * @param {function} fn The function to call if the service is not enabled
+   * @returns {boolean} true if the function was called; false if the service has
+   * already been enabled.
+   */
+  _unlessEnabled(fn) {
+    if (this._enabled) {
+      return false;
+    } else {
+      fn();
+      return true;
+    }
+  }
 }
diff --git a/src/Service.js b/src/Service.js
@@ -89,10 +89,10 @@ export default class Service {
   }
 
   /**
-   * Returns a map of ID's to slots.
+   * Returns a map of IDs to slots.
    *
    * @experimental
-   * @returns {object} The map of ID's to slots.
+   * @returns {Object} The map of IDs to slots.
    */
   getSlotIdMap() {
     return Object.assign({}, this._slotIdMap);
@@ -111,7 +111,7 @@ export default class Service {
    *
    * @param {string} eventType A string representing the type of event generated
    * by GPT. Event types are case sensitive.
-   * @param {function(event: object)} listener Function that takes a single event object argument.
+   * @param {function(event: Object)} listener Function that takes a single event object argument.
    * @returns {Service} The service object on which the method was called.
    */
   addEventListener(eventType, listener) {
@@ -125,7 +125,7 @@ export default class Service {
    * specified event object.
    *
    * @param {string} eventType The event type to fire.
-   * @param {object} event The event object to pass to listeners.
+   * @param {Object} event The event object to pass to listeners.
    * @private
    */
   _fireEvent(eventType, event) {

diff --git a/src/Size.js b/src/Size.js
@@ -46,7 +46,7 @@ export default class Size {
    * Returns whether the {@link Size} is empty.
    *
    * @experimental
-   * @type {boolean} True if the size is empty.
+   * @type {boolean} true if the size is empty.
    */
   isEmpty() {
     return !(this.width * this.height);

diff --git a/src/Slot.js b/src/Slot.js
@@ -5,6 +5,7 @@ import SlotOnloadEvent from './events/SlotOnloadEvent';
 import SlotRenderEndedEvent from './events/SlotRenderEndedEvent';
 import SlotVisibilityChangedEvent from './events/SlotVisibilityChangedEvent';
 import SlotId from './SlotId';
+import TargetingMap from './TargetingMap';
 
 /**
  * Slot is an object representing single ad slot on a page.
@@ -25,7 +26,7 @@ export default class Slot {
     this._sizes = GeneralSize.toSizes(size);
     this._services = [];
     this._categoryExclusions = [];
-    this._targeting = {};
+    this._targeting = new TargetingMap();
     this._attributes = {};
     this._clickUrl = null;
     this._responseInformation = null;
@@ -187,7 +188,22 @@ export default class Slot {
    * array if there is no such key.
    */
   getTargeting(key) {
-    return this._targeting[key] || [];
+    return this._targeting.get(key);
+  }
+
+  /**
+   * Sets a custom targeting parameter for this slot. Calling this method
+   * multiple times for the same key will overwrite old values. Values set here
+   * will overwrite targeting parameters set on service level. These keys are
+   * defined in your {@link DFP} account.
+   *
+   * @param {string} key Targeting parameter key.
+   * @param {string|!Array<string>} value Targeting parameter value or array of values.
+   * @returns {Slot} The {@link Slot} object on which the method was called.
+   */
+  setTargeting(key, value) {
+    this._targeting.set(key, value);
+    return this;
   }
 
   /**
@@ -197,25 +213,26 @@ export default class Slot {
    * @returns {!Array<string>} Array of targeting keys. Ordering is undefined.
    */
   getTargetingKeys() {
-    return Object.keys(this._targeting);
+    return this._targeting.keys();
   }
 
   /**
-   * Sets a custom targeting parameter for this slot. Calling this method
-   * multiple times for the same key will overwrite old values. Values set here
-   * will overwrite targeting parameters set on service level. These keys are
-   * defined in your {@link DFP} account.
+   * Returns the current targeting key-value dictionary.
+   *
+   * @experimental
+   * @returns {Object<string, Array<string>>} the targeting map.
+   */
+  getTargetingMap() {
+    return this._targeting.all();
+  }
+
+  /**
+   * Clears all custom slot-level targeting parameters for this slot.
    *
-   * @param {string} key Targeting parameter key.
-   * @param {string|!Array<string>} value Targeting parameter value or array of values.
    * @returns {Slot} The {@link Slot} object on which the method was called.
    */
-  setTargeting(key, value) {
-    if (Array.isArray(value)) {
-      this._targeting[key] = value;
-    } else {
-      this._targeting[key] = [value];
-    }
+  clearTargeting() {
+    this._targeting.clear();
     return this;
   }
 
@@ -239,26 +256,6 @@ export default class Slot {
     return this;
   }
 
-  /**
-   * Clears all custom slot-level targeting parameters for this slot.
-   *
-   * @returns {Slot} The {@link Slot} object on which the method was called.
-   */
-  clearTargeting() {
-    this._targeting = {};
-    return this;
-  }
-
-  /**
-   * Returns the current targeting key-value dictionary.
-   *
-   * @experimental
-   * @returns {Object<string, Array<string>>} the targeting map.
-   */
-  getTargetingMap() {
-    return Object.assign({}, this._targeting);
-  }
-
   /**
    * Adds a service to this slot.
    *

diff --git a/src/TargetingMap.js b/src/TargetingMap.js
@@ -0,0 +1,69 @@
+/**
+ * Manages setting and retrieving targeting.
+ *
+ * @experimental
+ */
+export default class TargetingMap {
+  /**
+   * Creates a new {@link TargetingMap} instance.
+   */
+  constructor() {
+    this._targeting = {};
+  }
+
+  /**
+   * Sets custom targeting parameters for a given key.
+   *
+   * @param {string} key Targeting parameter key.
+   * @param {string|!Array<string>} value Targeting parameter value or array of values.
+   */
+  set(key, value) {
+    if (Array.isArray(value)) {
+      this._targeting[key] = value;
+    } else {
+      this._targeting[key] = [value];
+    }
+  }
+
+  /**
+   * Returns a specific targeting parameter that has been set.
+   *
+   * @param {string} key The targeting key to look for.
+   * @returns {!Array<string>} The values associated with this key, or an empty
+   * array if there is no such key.
+   */
+  get(key) {
+    return this._targeting[key] || [];
+  }
+
+  /**
+   * Returns the list of all targeting keys that have been set.
+   *
+   * @returns {!Array<string>} Array of targeting keys. Ordering is undefined.
+   */
+  keys() {
+    return Object.keys(this._targeting);
+  }
+
+  /**
+   * Clears custom targeting parameters for a given key.
+   *
+   * @param {string} [key] Targeting parameter key.
+   */
+  clear(key) {
+    if (key == null) {
+      this._targeting = {};
+    } else {
+      delete this._targeting[key];
+    }
+  }
+
+  /**
+   * Returns the current targeting key-value dictionary.
+   *
+   * @returns {Object<string, Array<string>>} the targeting map.
+   */
+  all() {
+    return Object.assign({}, this._targeting);
+  }
+}