1
\$\begingroup\$

I'm working on a custom (self-hosted) ads solution that allows the users to create ad units (by uploading the banner image and target URL) and get the ad display code. A JSON response is returned (by the endpoint) for the requested ad. As of now, here is what I do to get and display the ad on the page where ad code is added:

JSON output at the endpoint:

{
    "success": true,
    "ad_url": "https:\/\/adlandingpage.com",
    "ad_banner": "http:\/\/localhost\/ads\/complete\/uploads\/banners\/add-banner.gif"
}

Content of rw-ads.js:

var getAds = function(ad_id, callback) {
  url = 'http://localhost/ads/complete/index.php/ads/adjson/'+ad_id+'/';
  var xhr = new XMLHttpRequest();
  xhr.open('get', url, true);
  xhr.responseType = 'json';
  xhr.onload = function() {
    var status = xhr.status;
    if(status == 200) {
      callback(null, xhr.response);
    } else {
      callback(status);
    }
  };
  xhr.send();
};

Ad code to be added to a page:

<div id="ad1"></div>
<script src="http://myadsplatform.tld/assets/js/rw-ads.js"></script>
<script>
    var ad_id = 1;
    getAds(ad_id, function(err, data) {
        if(data.success) {
           $('#ad1').html('<a href="'+data.ad_url+'"><img style="max-width: 100%;" src="'+data.ad_banner+'"></a>'); 
        }
    });
</script>

I need suggestions from experts. Am I doing this correctly or this can be improved? Thank you in advance for your help.

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

A few suggestions:

  1. A function declaration is a bit simpler and more robust regarding hoisting compared to a function expression which assigns an anonymous function to a variable.
  2. Supplying null as the first callback parameter on success is a bit quirky, either always supply the HTTP status or fail silently without calling the callback. If needed you can always introduce an error callback which might be called on xhr.onerror, too.
  3. The async parameter of xhr.open defaults to true and can be removed
  4. Your getAds function could be split into a generic getJSON function and a getAd function which supplies the specific ad URL to the former.
  5. Your current getAds callback requires knowledge about the specific JSON returned by your ad server. This logic should be part of your "rw-ads.js" library and e.g. become part of the new getAd function.

Exemplary code:

function getJSON(url, success) {
  var xhr = new XMLHttpRequest();
  xhr.open('get', url);
  xhr.responseType = 'json';
  xhr.onload = function() {
    if (xhr.status === 200) {
      success(xhr.response);
    }
  };
  xhr.send();
};

function getAd(id, success) {
  getJSON('http://localhost/ads/complete/index.php/ads/adjson/' + id, function(data) {
    if (data.success) {
      success('<a href="' + data.ad_url + '"><img style="max-width: 100%;" src="' + data.ad_banner + '"></a>');
    }
  });
}

getAd(1, function(html) {
  document.getElementById('ad1').innerHTML = html;
}

General considerations:

Since you don't rely on an external ad network but a self-hosted solution, you might want to inject your ads into your HTML document on the server side, without the asynchronous client side ajax request.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your input. Regarding your last paragraph, I want the users of this script to embed ad code on other sites as well. I'm going to try the functions you wrote. Thanks again. \$\endgroup\$
    – Rehmat
    Commented Apr 17, 2017 at 13:32

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.