1
\$\begingroup\$

I have this code that requests the graph API from Facebook to respond with a shared count for a particular URL (could be multiple on the page). Graph returns a full number (135934) and I take that number and abbreviate it into more friendly numbers (135K). Basically I'm looking for any help on improving the code to be more concise or if there's a better way to do something then to learn how.

  <div>
      <section data-url="url-to-this-article">
          <h3> Some Article </h3>
         <div class="box-counter"></div> 
      </section>
      <section data-url="url-to-this-article2">
          <h3> Some Article 2</h3>
         <div class="box-counter"></div> 
      </section>
      <section data-url="url-to-this-article3">
          <h3> Some Article 3</h3>
         <div class="box-counter"></div> 
      </section>
  </div>

<script>
    jQuery(function(){
            var theShares=[],
            theNewShares=[];
            var numb;
            if(typeof jQuery==='undefined'){
                console.log('need jQuery');
            }else{
                var c = document.getElementsByClassName('box-counter');
                numb = c.length;
                for (var i = 0; i < numb; i++) {
                    var tempURL = c[i].parentNode.attributes['data-url'].nodeValue;
                    jQuery.getJSON('http://graph.facebook.com/'+tempURL,function(data) {
                        theShares.push(data.shares);
                        insertDuring();
                    });
                }
            }
            function insertDuring(){
                if(numb == theShares.length){
                    var c = document.getElementsByClassName('box-counter');
                    for (var i = 0; i < numb; i++) {
                        var myString = theShares[i].toString();
                        if(theShares[i] >= 1000){
                            if(theShares[i] >= 10000){
                                if(theShares[i] >= 100000){
                                    if(theShares[i] >= 1000000){
                                        var diff = myString.length - 6;

                                        abridgeNumbers(myString,diff,"m",i,false);
                                    }else{
                                        abridgeNumbers(myString,3,"k",i,false);                                 
                                    }
                                }else{
                                    abridgeNumbers(myString,2,"k",i,false);
                                }
                            }
                            else{
                                abridgeNumbers(myString,2,"k",i,true);                          
                            }
                        }else{
                            theNewShares[i] = theShares[i];
                        }
                        c[i].innerHTML = theNewShares[i];
                    }
                }
            }
            function abridgeNumbers(theString,theLimit,theAbbrev,i,k){
                for (var j = 0; j < theLimit; j++) {
                    if(typeof theNewShares[i] === 'undefined'){
                        theNewShares[i] = theString.charAt(j);
                    }else{
                        if(k == true){
                            theNewShares[i] = theNewShares[i] +"."+ theString.charAt(j);
                        }else{
                            theNewShares[i] = theNewShares[i] + theString.charAt(j);
                        }
                    }
                };
                theNewShares[i] = theNewShares[i] + theAbbrev;
            }
        });
</script>
\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to CR! I hope you get great reviews! \$\endgroup\$ – Mathieu Guindon Apr 24 '15 at 0:01
1
\$\begingroup\$

Here is another solution, built in pure JS as you asked.
(I didn't reproduce the (unchanged) HTML part, so it's easier to focus on it)

<script>
var counters = document.getElementsByClassName('box-counter');
for (var i = 0, n = counters.length; i < n; i++) {
    var element = counters[i],
        XHR = new XMLHttpRequest();
    element.innerHTML = 'unknown';
    XHR.onreadystatechange = function() {
        if (XHR.readyState == 4 && XHR.status == 200) {
            element.innerHTML = friendlyExpress(
                JSON.parse(responseText).shares
            );
        }
    }
    XHR.open(
        'GET', 
        'http://graph.facebook.com/'
        + counters[i].parentNode.attributes['data-url'].value,
        /*async*/true);
    XHR.send();
}
function friendlyExpress(x, legend, power) {
    legend |= 'okmgpt';
    power |= 1024;
    var powerLimit = legend.length - 1,
        powerIndex = 0;
    while (x > (power - 1) && powerIndex < powerLimit) {
      x /= power;
      powerIndex++;
    }
    return Math.round(size) + legend[powerIndex];
}
</script>

There are some points I think I must explain to be clear:

  • regarding the whole code: since we no longer use jQuery, there is nothing to isolate this widget nor to ensure it waits loading complete to work: it is up to you to decide how to organize that depending on the other HTML and JS parts in your real application.

  • regarding the process strategy: I realized you didn't finally get anything else than displaying the shares numbers in each .box-counter elements.
    So I dramatically simplified it, since we don't need intermediate variables like theShares, theNewShares and so on

  • regarding Facebook Graph API: I'm not familiar with it at all, but I wanted to know a little, and I was surprised I couldn't find any reference to a member like shares in any returned object!
    Are you sure of what you're doing with it? May be you'll find this post of interest.
    Anyway I didn't changed anything to the way you used about that (and obviously couldn't test with true URLs).
  • regarding the share numbers conversion: BTW I seised the opportunity to enhance it a little (it's a kind of fantasy :-), so it may be reused in many other contexts. It 's why I keeped it as a separate function.

For the rest, I hope it is self-explanatory, but feel free to ask for precisions.

\$\endgroup\$
  • \$\begingroup\$ Thanks a ton cFreed, you're review and changes really are teaching me a ton. I marked this one as correct now (hopefully that gives you extra rep). \$\endgroup\$ – RenaissanceProgrammer May 7 '15 at 20:22
  • \$\begingroup\$ Hey @cFreed, just wanted to let you know there were a few things wrong with this code, I had fixed up some myself, but also had some assistance with the onreadystatchange event: stackoverflow.com/questions/30222699/… \$\endgroup\$ – RenaissanceProgrammer May 13 '15 at 18:52
  • \$\begingroup\$ Oops! Sorry if I posted with some errors, and thanks to have informed me. Could you point me to what was culpry? TIA. (anyway, I will have a look at the post you mentioned) \$\endgroup\$ – cFreed May 14 '15 at 0:41
  • \$\begingroup\$ No: finally don't answer :-) I just discovered that the other post was just the following of this one, and explains what issue you had with my code. Also I realize why I didn't pay sufficient attention to that point: without any real urls available, I tested a version where I had dropped the XHR part! So, again, thanks for the feedback. \$\endgroup\$ – cFreed May 14 '15 at 0:48
  • \$\begingroup\$ no worries, also I had been wondering about this legend |= 'okmgpt';, as it is it sets legend equal to 0, I had changed it to legend = 'okmgpt'; and then I was curious about the abbreviation usage, what do o,k,m,g,p, and t represent? I had changed mine to legend = ' kmbtg'; ( "space" for when less than thousands, k for thousands, m for millions, b for billions, t for trillions and g for gazillions (as a joke because when will an article ever get more than trillion shares)) \$\endgroup\$ – RenaissanceProgrammer May 14 '15 at 18:56
3
\$\begingroup\$

My first remark is that you're explicitely requiring jQuery for your code to execute but only use it for jQuery.getJSON(), while it may be used elsewhere.

So instead of:

        } else {
        var c = document.getElementsByClassName('box-counter');
        numb = c.length;
        for (var i = 0; i < numb; i++) {
            var tempURL = c[i].parentNode.attributes['data-url'].nodeValue;
            jQuery.getJSON('http://graph.facebook.com/'+tempURL,function(data) {
                theShares.push(data.shares);
                insertDuring();
            });
        }
    }

you may simply write:

        } else {
        jQuery('.box-counter').each( function(i) {
            jQuery.getJSON(
                'http://graph.facebook.com/' + this.parent().data('url'),
                function(data) {
                  theShares.push(data.shares);
                  insertDuring();
                }
            );
        }
    }

You may also replace this:

function insertDuring(){
    if(numb == theShares.length){
        var c = document.getElementsByClassName('box-counter');
        for (var i = 0; i < numb; i++) {
            // your internal code here...
        }
    }
}

by this simpler code:

function insertDuring(){
    jQuery('.box-counter').each( function(i) {
        // your internal code here, unchanged...
        this.innerHTML = theNewShares[i];
    }
}

Now regarding your abridgeNumbers() function:

function abridgeNumbers(theString,theLimit,theAbbrev,i,k){
    for (var j = 0; j < theLimit; j++) {
        if(typeof theNewShares[i] === 'undefined'){
            theNewShares[i] = theString.charAt(j);
        } else {
            if(k == true){
                theNewShares[i] = theNewShares[i] +"."+theString.charAt(j);
            } else {
                theNewShares[i] = theNewShares[i] + theString.charAt(j);
            }
        }
    };
    theNewShares[i] = theNewShares[i] + theAbbrev;
}

You can simplify the code using the ternary operator, so improving readability, like this:

function abridgeNumbers(theString, theLimit, theAbbrev, i, k) {
    for (var j = 0; j < theLimit; j++) {
        if(typeof theNewShares[i] === 'undefined') {
            theNewShares[i] = theString.charAt(j);
        } else {
            theNewShares[i] += (k ? "." :"") + theString.charAt(j);
        }
    }
    theNewShares[i] += theAbbrev;
}

(BTW you may notice that the ";" is not needed at the end of the for block)

In a more advanced way you may also increase performance. In the code above, each step executes the test about theNewShares[i], while it is useful only once. So you may deport it before the loop, using a short notation which ensures to define theNewShares[i] as empty string when it is yet undefined.

function abridgeNumbers(theString, theLimit, theAbbrev, i, k) {
    theNewShares[i] |= '';
    for (var j = 0; j < theLimit; j++) {
        theNewShares[i] += (k ? "." :"") + theString.charAt(j);
    }
    theNewShares[i] += theAbbrev;
}

Finally beyond those purely technical aspects you may look at my second answer below about how to enhance the "conversion" strategy to get friendly representation of the numbers.

\$\endgroup\$
  • \$\begingroup\$ Thanks for providing this review, sorry it took me a little while to accept it and give you that rep back! Really appreciate all your points. Here's something I wanted to point out, I only used jQuery because of the simplicity of the getJSON call, I actually prefer NOT to use jQuery when possible, but I also didn't have the time to write the JSON call using pure JS. If you have the time and feel up to it, can you provide an additional review using pure JS (up to you if you want to write getJSON over in pure JS or use it like I did. \$\endgroup\$ – RenaissanceProgrammer May 5 '15 at 19:25
2
\$\begingroup\$

Here is a second answer, which I decided to make distinct since the previous one focused on coding aspect, while this one proposes a totally different, pretty shorter, strategy for obtaining friendly representation of the numbers.

BTW I also suppressed what I seemed useless: each step of the initial loop (jQuery.getJSON()) called the insertDuring() function, but this one was working only under the if(numb == theShares.length) condition, so waiting for the whole loop executed.

So I merely suppressed the insertDuring() function and executed its (shortened) content after the initial loop. Maybe I'm omitting something more subtle: feel free to correct me.

You may also look at this fiddle to see an example running.

<div>
    <section data-url="url-to-this-article">
        <h3> Some Article </h3>
        <div class="box-counter"></div> 
    </section>
    <section data-url="url-to-this-article2">
        <h3> Some Article 2</h3>
        <div class="box-counter"></div> 
    </section>
    <section data-url="url-to-this-article3">
        <h3> Some Article 3</h3>
        <div class="box-counter"></div> 
    </section>
</div>

<script>
    jQuery(function(){
        var theShares=[],
            theNewShares=[],
            legend='okmg',
            powerLimit=legend.length - 1;
        if(typeof jQuery==='undefined'){
            console.log('need jQuery');
        } else {
            jQuery('.box-counter').each( function(i) {
                jQuery.getJSON(
                    'http://graph.facebook.com/' + this.parent().data('url'),
                    function(data) {
                      theShares.push(data.shares);
                    }
                );
            }
            var numb = theShares.length;
            for (var i = 0; i < numb; i++) {
                var number = theShares[i], powerIndex = 0;
                while (number > 1023 && powerIndex < powerLimit) {
                  number /= 1024;
                  powerIndex++;
                }
                this.innerHTML = Math.round(number) + legend[powerIndex];
            }
        }
    });
</script>

Regarding the conversion strategy, I added the legend variable which mentions the units we want to use: the interpretation stops at the maximum unit stated, and bigger numbers remain expressed in this unit. What I didn't pay attention to is the fact that in the original version you wanted a decimal part: you might work around the final Math.round(number) to manage this.

In the other hand it is pretty simple to choose the "dimension", e.g. replacing 'okmg' by 'okmgpt', without any other change needed for the code to work.
The process itself is hopefully self-explanatory.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.