r72759 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r72758‎ | r72759 | r72760 >
Date:21:35, 10 September 2010
Author:adam
Status:resolved (Comments)
Tags:
Comment:
Implimenting weighted banner picking
Modified paths:
  • /trunk/extensions/CentralNotice/newCentralNotice.js (modified) (history)

Diff [purge]

Index: trunk/extensions/CentralNotice/newCentralNotice.js
@@ -36,6 +36,7 @@
3737 if ( timestamp ) {
3838 listURL = "TBD"
3939 } else {
 40+ // http://geoiplookup.wikimedia.org/
4041 var geoLocation = 'US'; // Hard-coding for now
4142 var bannerListPage = 'Special:BannerListLoader?language='+wgContentLanguage+'&project='+wgNoticeProject+'&location='+geoLocation;
4243 //centralized version:
@@ -45,19 +46,40 @@
4647 var request = $.ajax( {
4748 url: bannerListURL,
4849 dataType: 'json',
49 - success: function( data ) {
50 - $.centralNotice.fn.chooseBanner( data );
51 - }
 50+ success: $.centralNotice.fn.chooseBanner
5251 } );
5352 },
5453 'chooseBanner': function( bannerList ) {
55 - // pick a banner based on logged-in status and geotargetting
56 - var bannerHTML = bannerList[0].html;
57 - $.centralNotice.fn.displayBanner( bannerHTML );
 54+ // convert the json object to a true array
 55+ bannerList = Array.prototype.slice.call( bannerList );
 56+
 57+ // Make sure there are some banners to choose from
 58+ if ( bannerList.length == 0 ) return false;
 59+
 60+ var groomedBannerList = [];
 61+
 62+ for( var i = 0; i < bannerList.length; i++ ) {
 63+ // only include this banner if it's inteded for the current user
 64+ if( ( wgUserName ? bannerList[i].display_account == 1 : bannerList.display_anon == 1 ) ) {
 65+ // add the banner to our list once per weight
 66+ for( var j=0; j < bannerList[i].weight; j++ ) {
 67+ groomedBannerList.push( bannerList[i] );
 68+ }
 69+ }
 70+ }
 71+
 72+ // return if there's nothing left after the grooming
 73+ if( groomedBannerList.length == 0 ) return false;
 74+ // load a random banner from our groomed list
 75+
 76+ $.centralNotice.fn.loadBanner(
 77+ groomedBannerList[ Math.floor( Math.random() * groomedBannerList.length ) ].name
 78+ );
5879 },
5980 'displayBanner': function( bannerHTML ) {
6081 // inject the banner html into the page
61 - $( '#siteNotice' ).prepend('<div id="centralnotice" class="'+(wgNoticeToggleState ? 'expanded' : 'collapsed')+'">'+bannerHTML+'</div>');
 82+ $( '#siteNotice' )
 83+ .prepend( '<div id="centralnotice" class="' + ( wgNoticeToggleState ? 'expanded' : 'collapsed' ) + '">' + bannerHTML + '</div>' );
6284 },
6385 'getQueryStringVariables': function() {
6486 document.location.search.replace( /\??(?:([^=]+)=([^&]*)&?)/g, function () {

Follow-up revisions

RevisionCommit summaryAuthorDate
r73010Followup to r72759 - implimenting Roan's suggestionsadam19:39, 14 September 2010

Comments

#Comment by Catrope (talk | contribs)   18:03, 14 September 2010
+				for( var i = 0; i < bannerList.length; i++ ) {
+					// only include this banner if it's inteded for the current user
+					if( ( wgUserName ? bannerList[i].display_account == 1 : bannerList.display_anon == 1 ) ) {
+						// add the banner to our list once per weight
+						for( var j=0; j < bannerList[i].weight; j++ ) {
+							groomedBannerList.push( bannerList[i] );
+						}
+					}
+				}

So if there's three banners, each with weight 80, you're building an array of 240 banners and selecting a random one from that. Instead of building such a large structure and immediately tearing it down again, you could quite simply calculate which banner to use, with something along the lines of:

var totalWeight = 0;
for ( each banner )
    totalWeight += banner.weight;

var index = Math.floor( Math.random() * totalWeight );
var w = 0;
var selectedBanner = banners[0]; // paranoia
for ( each banner ) {
    w += banner.weight;
    if ( w > index ) {
        selectedBanner = banner;
        break;
    }
}
#Comment by Adammiller~mediawikiwiki (talk | contribs)   18:23, 14 September 2010

Whoa, yeah I was being idiotic there. I'll implement your suggestion.

Status & tagging log