r107407 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r107406‎ | r107407 | r107408 >
Date:18:51, 27 December 2011
Author:krinkle
Status:ok (Comments)
Tags:
Comment:
[mediawiki.js] function order
* define before call
* Follows-up r107402, r107405
Modified paths:
  • /trunk/phase3/resources/mediawiki/mediawiki.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/mediawiki/mediawiki.js
@@ -521,6 +521,109 @@
522522 }
523523
524524 /**
 525+ * Automatically executes jobs and modules which are pending with satistifed dependencies.
 526+ *
 527+ * This is used when dependencies are satisfied, such as when a module is executed.
 528+ */
 529+ function handlePending( module ) {
 530+ var j, r;
 531+
 532+ try {
 533+ // Run jobs who's dependencies have just been met
 534+ for ( j = 0; j < jobs.length; j += 1 ) {
 535+ if ( compare(
 536+ filter( 'ready', jobs[j].dependencies ),
 537+ jobs[j].dependencies ) )
 538+ {
 539+ if ( $.isFunction( jobs[j].ready ) ) {
 540+ jobs[j].ready();
 541+ }
 542+ jobs.splice( j, 1 );
 543+ j -= 1;
 544+ }
 545+ }
 546+ // Execute modules who's dependencies have just been met
 547+ for ( r in registry ) {
 548+ if ( registry[r].state === 'loaded' ) {
 549+ if ( compare(
 550+ filter( ['ready'], registry[r].dependencies ),
 551+ registry[r].dependencies ) )
 552+ {
 553+ execute( r );
 554+ }
 555+ }
 556+ }
 557+ } catch ( e ) {
 558+ // Run error callbacks of jobs affected by this condition
 559+ for ( j = 0; j < jobs.length; j += 1 ) {
 560+ if ( $.inArray( module, jobs[j].dependencies ) !== -1 ) {
 561+ if ( $.isFunction( jobs[j].error ) ) {
 562+ jobs[j].error( e, module );
 563+ }
 564+ jobs.splice( j, 1 );
 565+ j -= 1;
 566+ }
 567+ }
 568+ }
 569+ }
 570+
 571+ /**
 572+ * Adds a script tag to the body, either using document.write or low-level DOM manipulation,
 573+ * depending on whether document-ready has occured yet.
 574+ *
 575+ * @param src String: URL to script, will be used as the src attribute in the script tag
 576+ * @param callback Function: Optional callback which will be run when the script is done
 577+ */
 578+ function addScript( src, callback ) {
 579+ var done = false, script;
 580+ if ( ready ) {
 581+ // jQuery's getScript method is NOT better than doing this the old-fashioned way
 582+ // because jQuery will eval the script's code, and errors will not have sane
 583+ // line numbers.
 584+ script = document.createElement( 'script' );
 585+ script.setAttribute( 'src', src );
 586+ script.setAttribute( 'type', 'text/javascript' );
 587+ if ( $.isFunction( callback ) ) {
 588+ // Attach handlers for all browsers (based on jQuery.ajax)
 589+ script.onload = script.onreadystatechange = function() {
 590+
 591+ if (
 592+ !done
 593+ && (
 594+ !script.readyState
 595+ || /loaded|complete/.test( script.readyState )
 596+ )
 597+ ) {
 598+
 599+ done = true;
 600+
 601+ // Handle memory leak in IE
 602+ script.onload = script.onreadystatechange = null;
 603+
 604+ callback();
 605+
 606+ if ( script.parentNode ) {
 607+ script.parentNode.removeChild( script );
 608+ }
 609+
 610+ // Dereference the script
 611+ script = undefined;
 612+ }
 613+ };
 614+ }
 615+ document.body.appendChild( script );
 616+ } else {
 617+ document.write( mw.html.element(
 618+ 'script', { 'type': 'text/javascript', 'src': src }, ''
 619+ ) );
 620+ if ( $.isFunction( callback ) ) {
 621+ // Document.write is synchronous, so this is called when it's done
 622+ callback();
 623+ }
 624+ }
 625+ }
 626+
 627+ /**
525628 * Executes a loaded module, making it ready to use
526629 *
527630 * @param module string module name to execute
@@ -606,53 +709,6 @@
607710 }
608711
609712 /**
610 - * Automatically executes jobs and modules which are pending with satistifed dependencies.
611 - *
612 - * This is used when dependencies are satisfied, such as when a module is executed.
613 - */
614 - function handlePending( module ) {
615 - var j, r;
616 -
617 - try {
618 - // Run jobs who's dependencies have just been met
619 - for ( j = 0; j < jobs.length; j += 1 ) {
620 - if ( compare(
621 - filter( 'ready', jobs[j].dependencies ),
622 - jobs[j].dependencies ) )
623 - {
624 - if ( $.isFunction( jobs[j].ready ) ) {
625 - jobs[j].ready();
626 - }
627 - jobs.splice( j, 1 );
628 - j -= 1;
629 - }
630 - }
631 - // Execute modules who's dependencies have just been met
632 - for ( r in registry ) {
633 - if ( registry[r].state === 'loaded' ) {
634 - if ( compare(
635 - filter( ['ready'], registry[r].dependencies ),
636 - registry[r].dependencies ) )
637 - {
638 - execute( r );
639 - }
640 - }
641 - }
642 - } catch ( e ) {
643 - // Run error callbacks of jobs affected by this condition
644 - for ( j = 0; j < jobs.length; j += 1 ) {
645 - if ( $.inArray( module, jobs[j].dependencies ) !== -1 ) {
646 - if ( $.isFunction( jobs[j].error ) ) {
647 - jobs[j].error( e, module );
648 - }
649 - jobs.splice( j, 1 );
650 - j -= 1;
651 - }
652 - }
653 - }
654 - }
655 -
656 - /**
657713 * Adds a dependencies to the queue with optional callbacks to be run
658714 * when the dependencies are ready or fail
659715 *
@@ -726,62 +782,6 @@
727783 }
728784
729785 /**
730 - * Adds a script tag to the body, either using document.write or low-level DOM manipulation,
731 - * depending on whether document-ready has occured yet.
732 - *
733 - * @param src String: URL to script, will be used as the src attribute in the script tag
734 - * @param callback Function: Optional callback which will be run when the script is done
735 - */
736 - function addScript( src, callback ) {
737 - var done = false, script;
738 - if ( ready ) {
739 - // jQuery's getScript method is NOT better than doing this the old-fashioned way
740 - // because jQuery will eval the script's code, and errors will not have sane
741 - // line numbers.
742 - script = document.createElement( 'script' );
743 - script.setAttribute( 'src', src );
744 - script.setAttribute( 'type', 'text/javascript' );
745 - if ( $.isFunction( callback ) ) {
746 - // Attach handlers for all browsers (based on jQuery.ajax)
747 - script.onload = script.onreadystatechange = function() {
748 -
749 - if (
750 - !done
751 - && (
752 - !script.readyState
753 - || /loaded|complete/.test( script.readyState )
754 - )
755 - ) {
756 -
757 - done = true;
758 -
759 - // Handle memory leak in IE
760 - script.onload = script.onreadystatechange = null;
761 -
762 - callback();
763 -
764 - if ( script.parentNode ) {
765 - script.parentNode.removeChild( script );
766 - }
767 -
768 - // Dereference the script
769 - script = undefined;
770 - }
771 - };
772 - }
773 - document.body.appendChild( script );
774 - } else {
775 - document.write( mw.html.element(
776 - 'script', { 'type': 'text/javascript', 'src': src }, ''
777 - ) );
778 - if ( $.isFunction( callback ) ) {
779 - // Document.write is synchronous, so this is called when it's done
780 - callback();
781 - }
782 - }
783 - }
784 -
785 - /**
786786 * Asynchronously append a script tag to the end of the body
787787 * that invokes load.php
788788 * @param moduleMap {Object}: Module map, see buildModulesString()

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r107402[mediawiki.js] code quality and clean up...krinkle18:25, 27 December 2011
r107405[mediawiki.js] use simple IIFE closure with object literal...krinkle18:48, 27 December 2011

Comments

#Comment by Nikerabbit (talk | contribs)   07:16, 28 December 2011

Do we really have to write JavaScript like C? main() last and variables declared on the first line.

#Comment by Krinkle (talk | contribs)   15:50, 28 December 2011

Regarding the variable declarations on top:

I have to admit I never invested much time in C, however I do know a fair bit about how JavaScript works and this is how JavaScript works whether we like it or not. And the quality of code tends to raise when programs are written the way the language they are written in works, not the way other languages the programmer likes work.

Prior to execution, JavaScript hoists all variable declarations within a function to the top of the function (block scope doesn't exist).

So, as examples:

function foo(){
  console.log( x ); // Throws exception: ReferenceError: x is not defined
}

function bar(){
  console.log( x ); // No exception, variable is already declared, just set to "undefined"

  var x = 5;
}

// bar() is actually quux()

function quux(){
  var bar;

  console.log( bar ); // No exception, variable is already declared, just set to "undefined"

  bar = 5;
}

Now in the above example it may seen trivial, but try the following example (based on example from "Crockford on JavaScript - Act III: Function the Ultimate")

var doStuff = function (foo) {
    for (var i = 0; i < foo.length; i += 1) {
        var bar = foo[i];
        for (var i = 0; i < bar.length; i += 1) {
            var quux = bar[i];
        }
    }
}

The above is an infinite loop.

So declaring them on top in my opinion:

  • Prevents re-using variables causing weird situations like the infinite loop
  • Gives quick overview of which variables are used in a function
  • Is what JavaScript will do internally anyway

Status & tagging log