SitePoint Sponsor

User Tag List

Page 1 of 2 12 LastLast
Results 1 to 25 of 33

Hybrid View

  1. #1
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)

    Separation of concerns (html, css, js)

    While watching Nicholas Zakas' Maintainable JavaScript talk at the Fluent 2012 conference, there was a very informative section in there about keeping JavaScript separate from the HTML, and other similar concerns of separation.

    You can see it from the 25:40 section of the video.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  2. #2
    Gre aus'm Pott gold trophysilver trophybronze trophy
    Pullo's Avatar
    Join Date
    Jun 2007
    Location
    Germany
    Posts
    5,941
    Mentioned
    215 Post(s)
    Tagged
    12 Thread(s)
    Hi Paul,

    Thanks for posting the link to that talk. I was reminded of a lot of things that I should be doing more often

    I do however have one question:

    At 27:02 in the video, he tells us to "Keep your HTML out of JavaScript", citing the reason that if there is something wrong with the layout, you shouldn't have to find the problem in a JS file.

    E.g. this is bad:

    Code JavaScript:
    var element = document.getElementById("container");
    element.innerHTML = "<div class=\"popup\"></div>"

    By way of a solution he proposes using some form of template (e.g. Moustache or Handlebars), or pulling back markup from the server and injecting it into the page.

    So far, so good, right?

    Well, the other day I saw you post this code, with which you can generate random numbers for a lottery.
    I noticed that if you pick nine or more numbers, the numbers overflow their container. So, I thought I'd change the markup and make the box expand to fit the numbers.
    As you might already guess, I couldn't find the markup in the HTML section of the fiddle, but instead found it buried deep in the JS. I had a quick play with it, but soon gave up owing to the confusing nature of the escaped backslashes, the inline CSS and the string concatenation.

    Code:
    container.innerHTML = '<div id="container' + idx + '" style="' + 'position:relative;' + 'width:160px;height:50px;' + 'font-family:verdana,arial,sans-serif;' + 'font-size:12px;' + 'color:#000000;' + 'background-color:#fffff0;' + 'text-align:center;' + 'border : 1px solid #000000">' + '<input type="button" id="play' + idx + '"' + 'value="' + buttonText + '" style="margin:5px">' + '<div id="result' + idx + '" style="' + 'width:150px;' + 'font-family:verdana,arial,sans-serif;' + 'font-size:12px;' + 'color:#000000">' + initialText + '<\/div><\/div>';
    document.body.appendChild(container);
    I know this code was only intended to demonstrate a specific functionality, but this got me to wondering, how we might apply what Nicholas Zakas tells us in his talk.

    I would be interested in your thoughts on this.

  3. #3
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by Pullo View Post
    I know this code was only intended to demonstrate a specific functionality, but this got me to wondering, how we might apply what Nicholas Zakas tells us in his talk.

    I would be interested in your thoughts on this.
    Removing the CSS from the JavaScript that's intended to be HTML, would be a good first step to things.
    Then, the remaining HTML could be placed in to the web page as hidden content, or in to a template file of some kind for later use.

    How do you feel about refactoring that code, as an example of how to apply such better practices?
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  4. #4
    Gre aus'm Pott gold trophysilver trophybronze trophy
    Pullo's Avatar
    Join Date
    Jun 2007
    Location
    Germany
    Posts
    5,941
    Mentioned
    215 Post(s)
    Tagged
    12 Thread(s)
    Quote Originally Posted by paul_wilkins View Post
    How do you feel about refactoring that code, as an example of how to apply such better practices?
    Sure!

    Quote Originally Posted by paul_wilkins View Post
    Removing the CSS from the JavaScript that's intended to be HTML, would be a good first step to things.

    Then, the remaining HTML could be placed in to the web page as hidden content, or in to a template file of some kind for later use.
    Ok, I've done this: http://jsfiddle.net/hibbard_eu/8R42e/
    As you can see, I've created a template div element, which I've hidden using CSS.
    I can then clone this with JS and manipulate it as required before inserting it into the page.
    The code could probably do with some refactoring, but does this seem like a good start?

    I've also used classes for the styling, as the dynamically generated elements are assigned varying ids, e.g.

    Code JavaScript:
    container.innerHTML = '<div id="container' + idx + ...

  5. #5
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by Pullo View Post
    Sure!



    Ok, I've done this: http://jsfiddle.net/hibbard_eu/8R42e/
    As you can see, I've created a template div element, which I've hidden using CSS.
    I can then clone this with JS and manipulate it as required before inserting it into the page.
    The code could probably do with some refactoring, but does this seem like a good start?
    Yes, that's a good start.

    The following can be removed entirely:
    Code:
    template.style.display = 'block';
    because it is only the template section that is hidden. When it's copied out using cloneNode and renamed, it's no longer hidden.

    The template doesn't need to be appended to the page immediately after it has been read too.

    Code:
    container.innerHTML = '';
    
    var template = document.getElementById('template').cloneNode(true);
    container.appendChild(template);
    template.id = "container" + idx;
    template.className = "container";
    Instead, changes are better off being made before it has been added to the page, which helps to avoid performance issues too.

    Code javascript:
    var template = document.getElementById('template').cloneNode(true);
    template.id = "container" + idx;
    template.className = "container";
    ...
    container.appendChild(template);

    There's not much performance-wise to currently worry about, but I plan to take that appendChild down below the element updates and the event attachments.

    The section with the roll button can be updated now.

    Code:
    var button = template.getElementsByTagName("input")[0];
    button.id = "play" + idx;
    button.value = buttonText;
    ...
    var controlButton = document.getElementById("play" + idx);
    if (window.addEventListener) {
        controlButton.addEventListener("click", justOnce, false);
    } else if (window.attachEvent) {
        controlButton.attachEvent("onclick", justOnce);
    }
    The button variable can be renamed to something a but more expressive, to playButton.
    And that event code is too complex for its own good. We can either move the cross-compatibility addEvent code out to a separate function, or we can use a more simple convention instead. Let's go with the simple and use onclick.

    We can also entirely get rid of the controlButton variable too.

    Code javascript:
    var playButton = template.getElementsByTagName("input")[0];
    playButton.id = "play" + idx;
    playButton.value = buttonText;
    playButton.onclick = justOnce;

    That's much better.

    There's a minor conflict after that in the justOnce function because it is expecting the controlButton global variable, but that's easily fixed, by using the this keyword instead.
    And why is the b variable there in the justOnce function? Normally it would be evt or some other similar event keyword, but the variable is not used at all in this function, so that variable can be removed too.

    Before:
    Code:
    function justOnce(b) {
        controlButton.blur();
        ...
    }
    After:
    Code javascript:
    function justOnce() {
        var button = this;
        button.blur();
        ...
    }

    After these changes, we now are left with a structure where the template is obtained, changes are made to the content of that template, and at the end of things the template is placed inside of the container.

    Code javascript:
    function lotto() {
        // functions
        ...
     
        var container = document.getElementById('container');
     
        // updates to the template
        ...
     
        container.innerHTML = '';
        appendChild(template);
    }

    Now because I'm lazy developer, I'm going to have JSLint help me with tidying up the rest of this code, which results in the following code: http://jsfiddle.net/pmw57/8R42e/15/

    Now that the code is structured and safe, there are many other improvements that should be made to it as well.
    • The large list of variables means that we should break the main lotto function up in to separate parts
    • I want to create a single global lotto object, which we can use to initialize button text values and to act as the single interface for our script
    • The numsort function can be much simpler, and achieve the same job
    • Most of the code should be broken up in to separate functions, dedicated to just one task


    And the list of improvements goes on
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  6. #6
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by paul_wilkins View Post
    Now that the code is structured and safe, there are many other improvements that should be made to it as well.
    • The large list of variables means that we should break the main lotto function up in to separate parts
    • I want to create a single global lotto object, which we can use to initialize button text values and to act as the single interface for our script
    • The numsort function can be much simpler, and achieve the same job
    • Most of the code should be broken up in to separate functions, dedicated to just one task
    I have to head off, but as something to look forward to, here's what the roll function looks like after having made a few improvements:

    Code javascript:
    function roll(button) {
        if (!validateRollingOptions(opts.pick, opts.from, opts.to)) {
            return false;
        }
     
        var draw = drawNumbers(opts.pick, opts.from, opts.to);
        draw.sort(numericSort);
        updateResult(draw.join(' '));
     
        setupNextRoll({
            target: button,
            totalRolls: opts.totalRolls,
            rollDelay: opts.rollDelay,
            callback: roll
        });
    }
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  7. #7
    Gre aus'm Pott gold trophysilver trophybronze trophy
    Pullo's Avatar
    Join Date
    Jun 2007
    Location
    Germany
    Posts
    5,941
    Mentioned
    215 Post(s)
    Tagged
    12 Thread(s)
    Hi Paul,

    I've just finished reading through your first refactoring of the code.
    Thank you very much for this, as writing maintainable code, which reflects current best practices is definitely something I could be better at. It is certainly very educational to see how somebody else would go about things.
    I should also get into the habit of using JS lint more often. I also ran the code through it so that I could better comprehend your final Fiddle and was quite surprised at the amount of warnings it spat out.
    Most of them were logical really, but these are all things which can potentially make your code less error prone in the long run.

    Quote Originally Posted by paul_wilkins View Post
    The numsort function can be much simpler, and achieve the same job
    Allow me:

    Code JavaScript:
    function numsort(n1, n2) {
      return (n1 - n2);
    }
     
    var draw = [19, 32, 18, 36, 4, 43, 5] ;
    console.log(draw.sort(numsort));
     
    // [4, 5, 18, 19, 32, 36, 43]

    I am looking forward to the second instalment. Things look as though they are shaping up quite nicely.

  8. #8
    SitePoint Wizard bronze trophy Jeff Mott's Avatar
    Join Date
    Jul 2009
    Posts
    1,278
    Mentioned
    18 Post(s)
    Tagged
    0 Thread(s)
    Here's an interesting trick that gets used far less often than it should.

    Code HTML4Strict:
    <script id="list-template" type="text/x-handlebars-template">
        <p>YUI is brought to you by:</p>
     
        <ul>
            {{#items}}
                <li><a href="{{url}}">{{name}}</a></li>
            {{/items}}
        </ul>
    </script>

    This way, your markup stays in your HTML files; there's no need for any backslash escaping; and the browser won't render this template because it's wrapped in a script tag.

    Then somewhere in your JavaScript (this uses YUI, but you get the gist):

    Code JavaScript:
    // Extract the template string and compile it into a reusable function.
    var template = Y.Handlebars.compile(Y.one('#list-template').getHTML());
     
    // Render the template to HTML using the specified data.
    var html = template({
        items: [
            {name: 'pie', url: 'http://pieisgood.org/'},
            {name: 'mountain dew', url: 'http://www.mountaindew.com/'},
            {name: 'kittens', url: 'http://www.flickr.com/search/?q=kittens'},
            {name: 'rainbows', url: 'http://www.youtube.com/watch?v=OQSNhk5ICTI'}
        ]
    });
    "First make it work. Then make it better."

  9. #9
    Gre aus'm Pott gold trophysilver trophybronze trophy
    Pullo's Avatar
    Join Date
    Jun 2007
    Location
    Germany
    Posts
    5,941
    Mentioned
    215 Post(s)
    Tagged
    12 Thread(s)
    Quote Originally Posted by Jeff Mott View Post
    Here's an interesting trick that gets used far less often than it should.
    Thanks for pointing that out, Jeff.
    I've been wanting to have a look at Handlebars for a while now (along with so much other cool stuff - where does the time go??).
    Maybe when we've worked through refactoring the code here, it would be possible to convert it to use Handlebars (and/or Moustache) as a final step.

  10. #10
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by Pullo View Post
    I am looking forward to the second instalment. Things look as though they are shaping up quite nicely.
    Thanks.

    Going through the code at http://jsfiddle.net/pmw57/8R42e/15/ there are more issues with it that was dealing with now.

    • It is tempting to define a global object for the code, by that is not required yet at this stage
    • the "choose your lottery format" piece needs to be moved out as configuration options
    • the configuration options will need to be provided through a global object for the code, so we now have a good reason to define the code as a global object
    • the playing variable can be renamed to something a bit more expressive, such as isRolling
    • timer doesn't seem to be needed, as we are not stopping the timed event at any stage
    • counter is for the number of times the numbers are rolled, which should be moved out to a configurable options object
    • idx and individually numbered identifier should not be required. Experiments with having multiple lotto rollers will need to be done in order to troubleshoot any remaining issues with that
    • container and template need to be configurable options
    • the play button and result sections should be not be retrieved from the page, but instead be assigned when the lotto section is being created
    • numsort should be replaced with much simpler code, and be renamed to numberSort
    • the roll function needs a much simpler structure, which should result inonly three functions being called. One for validation, one to get the drawn numbers and one to show them
    • the template modifications should be moved to a separate function
    • the check that getDocumentById exists should throw an error if it doesn't
    • the firstChild check shouldn't be needed due to the refactoring, but testing will confirm that
    • the onsubmit function should just pass the form element to an init function


    Those are the broad strokes of what needs to be done, which I'll temporarily attempt to work on from the ipad for a while.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  11. #11
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by paul_wilkins View Post
    Those are the broad strokes of what needs to be done, which I'll temporarily attempt to work on from the ipad for a while.
    The timer that is declared in the lotto function is used I only one place, and that is where the decision on whether to keep the rolling going on occurs.

    Code:
    timer = setTimeout(roll, 50);
    if (counter > 50) {
        clearTimeout(timer);
        isRolling = false;
        counter = 0;
    }
    By reorganizing that, we can remove the need for clearing the timer.

    But, I cannot do this with my ipad because when I head off to edit some code, I'm forced to reload this comment page when I return, which forces me to lose all that I have typed up before.

    The third time is the charm, time to admit defeat and do something else until at get back to a real computer.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  12. #12
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by paul_wilkins View Post
    The third time is the charm, time to admit defeat and do something else until at get back to a real computer.
    Okay now - removing the need for the timer.

    We can check if we need the timer before setting it. That will allow us to remove the need for the separate timer variable.

    Code javcascript:
    if (counter <= 50) {
        setTimeout(roll, 50);
    else {
        isRolling = false;
        counter = 0;
    }

    Those 50's each have different meanings, so we should place those in a config options area.

    Code javascript:
    var lotto = function (pick, from, to) {
        var opts = {
            totalRolls: 50,
            rollDelay: 50
        },
        ...

    After other refactorings have occurred to simplify things, we'll replace those function parameters with a single options object instead.

    The playing variable can be renamed to isRolling, and we can come back to that one later to decide if it's better as a variable, or a property on the button.

    buttonText and initalText can go in to the opts object:

    Code javascript:
    var opts = {
        buttonText: "Lotto Lucky Dip",
        initialText: "Your Lucky Numbers",
    ...
    playButton.value = opts.buttonText;
    ...
    resultDiv.innerHTML = opts.initialText;

    Let us now split up the roll function in to separate parts.

    First there is the validation code, which uses pick, from and to.

    Code:
    if (from >= to) {
        window.alert("from value must be less than to value");
        return false;
    }
    if ((to + 1) - from < pick) {
        window.alert("Error - You want " + pick + " numbers.\n\n" + "The range you have entered is from " + from + " to " + to + ".\n" + "This leaves " + (rng + 1) + " available random numbers.");
        return false;
    }
    Because both parts return false, we can use a standard validation technique of having an isValid variable at the start of the function, which gets set to false if anything fails, and then return isValid.

    Code javascript:
    function validateChoices(pick, from, to) {
        var isValid = true;
     
        if (from >= to) {
            window.alert("from value must be less than to value");
            isValid = false;
        }
        if ((to + 1) - from < pick) {
            window.alert("Error - You want " + pick + " numbers.\n\n" + "The range you have entered is from " + from + " to " + to + ".\n" + "This leaves " + (rng + 1) + " available random numbers.");
            isValid = false;
        }
     
        return isValid;
    }
    ...
    if (!validateChoices(pick, from, to)) {
        return false;
    }

    and the part of the roll function that draws the different numbers, can be extracted out to a drawNumbers function

    Code:
    function drawNumbers(pick, from, to) {
        var draw = [],
            rng = to - from,
            e = (rng + 1),
            i,
            j,
            number;
        isRolling = true;
    
        for (i = 0; i < pick; i += 1) {
            number = parseInt(from + Math.random() * e, 10);
            for (j = 0; j < pick; j) {
                if (number !== draw[j]) {
                    j += 1;
                } else {
                    number = parseInt(from + Math.random() * e, 10);
                    j = 0;
                }
            }
            draw[i] = number;
        }
        return draw;
    }
    ...
    var i,
        dum = "",
        disp,
        draw = drawNumbers(pick, from, to);
    That drawNumbers function can now be refactored, so that instead of rng and e variables, we can just use a delta variable.

    Code javascript:
     var draw = [],
        delta = to - from + 1, // inclusive of both ends

    and the random number functions we can move out to a separate getRandomInteger function

    Code javascript:
    function getRandomInteger(from, to) {
        var delta = to - from + 1; // inclusive of both ends
     
        return parseInt(from + Math.random() * delta, 10);
    }
    ...
    number = getRandomInteger(from, to);

    but we still need to simplify the nested loop that draws a new random number, which is currently:

    Code:
    for (i = 0; i < pick; i += 1) {
        number = getRandomInteger(from, to);
        for (j = 0; j < pick; j) {
            if (number !== draw[j]) {
                j += 1;
            } else {
                number = getRandomInteger(from, to);
                j = 0;
            }
        }
        draw[i] = number;
    }
    It may be easier to see what we need to do if we switch around the if/else statement

    Code:
    if (number === draw[j]) {
        number = getRandomInteger(from, to);
        j = 0;
    } else {
        j += 1;
    }
    So what we need to do, is to keep on picking numbers until we find one that has not already been picked.
    We have a couple of different options here. We could tidy up the existing structure, or we could come up with a completely different solution where we randomize an array of numbers and then pick the first n from that array.

    I'll leave off temporarily here to take care of some things, and consider those options.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  13. #13
    Gre aus'm Pott gold trophysilver trophybronze trophy
    Pullo's Avatar
    Join Date
    Jun 2007
    Location
    Germany
    Posts
    5,941
    Mentioned
    215 Post(s)
    Tagged
    12 Thread(s)
    I followed along with all of the refactorings, however one thing occurred to me.

    Quote Originally Posted by paul_wilkins View Post
    First there is the validation code, which uses pick, from and to.

    Code:
    if (from >= to) {
        window.alert("from value must be less than to value");
        return false;
    }
    if ((to + 1) - from < pick) {
        window.alert("Error - You want " + pick + " numbers.\n\n" + "The range you have entered is from " + from + " to " + to + ".\n" + "This leaves " + (rng + 1) + " available random numbers.");
        return false;
    }
    Because both parts return false, we can use a standard validation technique of having an isValid variable at the start of the function, which gets set to false if anything fails, and then return isValid.

    Code javascript:
    function validateChoices(pick, from, to) {
        var isValid = true;
     
        if (from >= to) {
            window.alert("from value must be less than to value");
            isValid = false;
        }
        if ((to + 1) - from < pick) {
            window.alert("Error - You want " + pick + " numbers.\n\n" + "The range you have entered is from " + from + " to " + to + ".\n" + "This leaves " + (rng + 1) + " available random numbers.");
            isValid = false;
        }
     
        return isValid;
    }
    ...
    if (!validateChoices(pick, from, to)) {
        return false;
    }
    If you move the validation into its own method, where should the call to validateChoices go?
    Ideally, you want it to be called once, when the user presses "Update Lucky Dip" and not, as is currently happening, when the user presses "Lotto Lucky Dip" to start the draw.
    However, as validateChoices is defined from within the lotto function, you cannot simply write:

    Code JavaScript:
    form.onsubmit = function () {
      var form = this,
      pick = Number(form.elements.pick.value) || 0,
      from = Number(form.elements.from.value) || 0,
      to = Number(form.elements.to.value) || 0;
     
      if (!validateChoices(pick, from, to)) {
        return false;
      }
     
      lotto(pick, from, to);
      return false;
    };

    This is where I get a bit confused with JS. In ruby you could write Lotto::validateChoices (Lotto being a class).

    Anyway, I moved this:

    Code JavaScript:
    if (!validateChoices(pick, from, to)) {
        return false;
    }

    to the top of the lotto function after the variable declarations, so at least it only gets called once.

    The downside of that, is that the function itself has no concept of the variable rng, so you have to write:

    Code JavaScript:
    window.alert("Error - You want " + pick + " numbers.\n\n" + "The range you have entered is from " + from + " to " + to + ".\n" + "This leaves " + ((to - from) + 1) + " available random numbers.");

    instead of:

    Code JavaScript:
    window.alert("Error - You want " + pick + " numbers.\n\n" + "The range you have entered is from " + from + " to " + to + ".\n" + "This leaves " + (rng + 1) + " available random numbers.");

    What are your thoughts on this?

  14. #14
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by Pullo View Post
    to the top of the lotto function after the variable declarations, so at least it only gets called once.

    The downside of that, is that the function itself has no concept of the variable rng
    My thoughts on this are that it's okay for the range variable to be calculated in a few different sections. If a third or more situations occur though, that is a good time to refactor things further.
    Alternatively, you can have a getRange() function which accepts to and from, and returns the calculated range to both areas.

    Whichever one you choose is a choice on your part as author, about which one you feel will result in a greater understanding for the person reading the code.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  15. #15
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by paul_wilkins View Post
    So what we need to do, is to keep on picking numbers until we find one that has not already been picked.
    We have a couple of different options here. We could tidy up the existing structure, or we could come up with a completely different solution where we randomize an array of numbers and then pick the first n from that array.
    It is tempting to randomize an array of values, but we don't know if people will choose to pick numbers from a large range of potential millions, so I'll go with the less brittle technique of checking if the chosen number already exists in the array.

    I've moved the inner loop out to a separate function called arrayHasNumber(), which has allowed me to use a while loop in there, which represents what we are wanting to achieve to a much more accurate degree.

    Code javascript:
    function drawNumbers(pick, from, to) {
        var draw = [],
            i,
            number;
     
        for (i = 0; i < pick; i += 1) {
            do {
                number = getRandomInteger(from, to);
            } while (arrayHasNumber(draw, number));
            draw[i] = number;
        }
        return draw;
    }

    With the arrayHasNumber function, I don't know if a persons web browser yet supports the Array.indexOf method, so we can check if that method exists before using it, and if not we can fall back to looping through the array.

    In fact, there's a better way of doing this, so here it is just in a code block, to show what was going to be used:

    Code:
    function arrayHasNumber(array, number) {
        // use native technique if available
        if (Array.indexOf) {
            return (array.indexOf(number) > -1);
        }
    
        // otherwise check manually
        var i,
            hasNumber = false;
        for (i = 0; i < array.length; i += 1) {
            if (number === array[i]) {
                hasNumber = true;
                break;
            }
        }
        return hasNumber;
    }
    A better way, is to provide compatibility code for the Array.indexOf method right from the start, which we can get from https://developer.mozilla.org/en-US/.../Array/indexOf

    Code javascript:
    if (!Array.prototype.indexOf) {
        Array.prototype.indexOf = function (searchElement /*, fromIndex */ ) {
            "use strict";
            if (this == null) {
                throw new TypeError();
            }
            var t = Object(this);
            var len = t.length >>> 0;
            if (len === 0) {
                return -1;
            }
            var n = 0;
            if (arguments.length > 1) {
                n = Number(arguments[1]);
                if (n != n) { // shortcut for verifying if it's NaN
                    n = 0;
                } else if (n != 0 && n != Infinity && n != -Infinity) {
                    n = (n > 0 || -1) * Math.floor(Math.abs(n));
                }
            }
            if (n >= len) {
                return -1;
            }
            var k = n >= 0 ? n : Math.max(len - Math.abs(n), 0);
            for (; k < len; k++) {
                if (k in t && t[k] === searchElement) {
                    return k;
                }
            }
            return -1;
        }
    }

    Now we can replace that arrayHasNumber function with:

    Code javascript:
    function arrayHasNumber(array, number) {
        return array.indexOf(number) > -1;
    }

    We might even remove the arrayHasNumber function and just put its code directly in the while loop, but that comes down to a choice of which is more expressive.

    Either this:
    Code javascript:
    } while (draw.indexOf(number) > -1);
    or that:
    Code javascript:
    } while (numberAlreadyExists(number, draw));

    I might go for the latter, but it depends too on the potential audience.

    The next part to refactor is the display of the drawn numbers.

    Code:
    for (i = 0; i < pick; i += 1) {
        disp = dum += (draw[i] + " ");
    }
    counter += 1;
    document.getElementById("result" + idx).firstChild.data = disp;
    The loop can be replaced with a simple join statement, so we don't need the dum variable.
    The resultDiv was defined earlier on, so we can use that here too instead of searching for it.
    And the disp variable is used only once, so we can place the join at the end of the assignment.

    So the above code is brought down to just the following:

    Code javascript:
    resultDiv.innerHTML = draw.join(' ');

    The last part of the roll function is about setting up the next roll:

    Code:
    counter += 1;
    if (counter <= opts.totalRolls) {
        setTimeout(roll, 50);
    } else {
        isRolling = false;
        counter = 0;
    }
    The isRolling variable being in the same place as resetting the counter helps me to realise that the isRolling variable isn't even needed. We can just check if the counter is greater than 0, so all isRolling parts can be removed, and the playButton onclick function can instead have this in there instead:

    Code javascript:
    if (counter > 0) {
        return false;
    }

    So let's move the remaining code to its own function called setupNextRoll()

    Code javascript:
    function setupNextRoll(totalRolls, rollDelay, callback) {
        counter += 1;
        if (counter <= totalRolls) {
            setTimeout(callback, rollDelay);
        } else {
            counter = 0;
        }    
    }
    ...
    setupNextRoll(opts.totalRolls, opts.rollDelay, roll);

    I'm passing the roll function at the end, because if we didn't do that then we would need the setupNextRoll function to refer to the roll function that is below it, which in turn refers to the function above it. Linting software complains about calls that go upwards, and the callback is a highly effective method to convey what is happening, so it solves a number of related issues.

    That should do us for this part of things in the roll function. It's now looking nice and expressive, where validate before drawing, sorting, and showing the numbers, thengo to the next roll.

    Code javascript:
    function roll() {
        if (!validateChoices(pick, from, to)) {
            return false;
        }
     
        var draw = drawNumbers(pick, from, to);
        draw.sort(numericSort);
        resultDiv.innerHTML = draw.join(' ');
     
        setupNextRoll(opts.totalRolls, opts.rollDelay, roll);
    }

    The resultDiv part could be moved out to a separate function so that we can more easily separate the action of rolling from the act of showing the content, but there are bigger fish to fry for now.

    Now that the roll function has been simplified we can move the function parameters to the opts object, which will help us to keep our configuration information all in the same consistent location.

    Code javascript:
    var lotto = function (options) {
            var opts = {
                buttonText: "Lotto Lucky Dip",
                initialText: "Your Lucky Numbers",
                pick: options.pick,
                from: options.from,
                to: options.to,
                totalRolls: 50,
                rollDelay: 50
            },
    ...
    if (!validateChoices(opts.pick, opts.from, opts.to)) {
    ...
    var draw = drawNumbers(opts.pick, opts.from, opts.to);
    ...
    var form = this;
    lotto({
        pick: Number(form.elements.pick.value) || 0,
        from: Number(form.elements.from.value) || 0,
        to: Number(form.elements.to.value) || 0
    });

    What we end up with is shown at http://jsfiddle.net/pmw57/8R42e/18/

    We are nearly ready now to init the lotto object using custom parameters, which will be delved in to about tomorrow.
    Last edited by paul_wilkins; Apr 29, 2013 at 05:00.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  16. #16
    Gre aus'm Pott gold trophysilver trophybronze trophy
    Pullo's Avatar
    Join Date
    Jun 2007
    Location
    Germany
    Posts
    5,941
    Mentioned
    215 Post(s)
    Tagged
    12 Thread(s)
    Wow! Good stuff!
    That is really well explained and easy to follow.

    I have a couple of questions:

    • In your opinion, at what point does it become feasible to start monkey patching objects, as opposed to rolling your own (admittedly more limited) solution
      E.g. when would you opt for dynamically extending the Array object with an indexof() method, as opposed to building this functionality into the arrayHasNumber function?

    • I didn't quite get why you need to pass roll as a callback to setupNextRoll.
      You could just as easily write:
      Code JavaScript:
      setTimeout(roll, rollDelay);
      Is this a stylistic thing or is there a situation when this might lead to unexpected behaviour?

    Look forward to the next instalment.

  17. #17
    Gre aus'm Pott gold trophysilver trophybronze trophy
    Pullo's Avatar
    Join Date
    Jun 2007
    Location
    Germany
    Posts
    5,941
    Mentioned
    215 Post(s)
    Tagged
    12 Thread(s)
    Howdy,

    So, I've just finished working through your latest set of refactorings.
    I like how you are applying the single responsibility principle (if you can call it that). Already the code is starting to become a lot more readable.

    I have some questions and some observations.
    I'll reply to your posts separately, so as to make things easier to follow.

    Quote Originally Posted by paul_wilkins View Post
    idx and individually numbered identifier should not be required. Experiments with having multiple lotto rollers will need to be done in order to troubleshoot any remaining issues with that
    This was bothering me when I did the initial separation of HTML and JS.
    Do you think you are ever likely to need multiple lotto rollers on the same page?

    Quote Originally Posted by paul_wilkins View Post
    container and template need to be configurable options
    the template modifications should be moved to a separate function
    That would be good.

    Quote Originally Posted by paul_wilkins View Post
    numsort should be replaced with much simpler code, and be renamed to numberSort
    See my previous post

    Quote Originally Posted by paul_wilkins View Post
    the check that getDocumentById exists should throw an error if it doesn't
    the firstChild check shouldn't be needed due to the refactoring, but testing will confirm that
    Why would you check for this?
    What purpose does this line actually serve?

    Code JavaScript:
    if ((document.getElementById && document.firstChild) && window.addEventListener || window.attachEvent) {

  18. #18
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by Pullo View Post
    This was bothering me when I did the initial separation of HTML and JS.
    Do you think you are ever likely to need multiple lotto rollers on the same page?
    It's not ordinarily likely, but I can see a use for it.
    • there may be a page may list different pre-defined lotto draws
    • or a demo page that shows how different types of settings work
    • or people may be able to save lotto settings that they like, as different rollers on a custom page


    Quote Originally Posted by Pullo View Post
    Why would you check for this?
    What purpose does this line actually serve?

    Code JavaScript:
    if ((document.getElementById && document.firstChild) && window.addEventListener || window.attachEvent) {
    It was a common technique with older web browsers, for example, with IE4 and earlier not supporting getElementById. It's not a check that we tend to use nowdays, but for the sake of completion I'll leave it in with an appropriately thrown exception.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  19. #19
    Gre aus'm Pott gold trophysilver trophybronze trophy
    Pullo's Avatar
    Join Date
    Jun 2007
    Location
    Germany
    Posts
    5,941
    Mentioned
    215 Post(s)
    Tagged
    12 Thread(s)
    Quote Originally Posted by paul_wilkins View Post
    It's not ordinarily likely, but I can see a use for it.
    • there may be a page may list different pre-defined lotto draws
    • or a demo page that shows how different types of settings work
    • or people may be able to save lotto settings that they like, as different rollers on a custom page
    Good point!

    Quote Originally Posted by paul_wilkins View Post
    It was a common technique with older web browsers, for example, with IE4 and earlier not supporting getElementById. It's not a check that we tend to use nowdays, but for the sake of completion I'll leave it in with an appropriately thrown exception.
    Ah ok. Thanks!

  20. #20
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by paul_wilkins View Post
    We are nearly ready now to init the lotto object using custom parameters, which will be delved in to about tomorrow.
    The next part to deal with is in terms of the idx variable, and the template side of things.

    These two lines can be removed.

    Code:
    template.id = "container" + idx;
    template.className = "container";
    The code that changes the id of the template isn't actually needed. All that's needed is to remove the id from the template part itself.

    Code javascript:
    template.removeAttribute('id');

    And for the className part, it's more appropriate for that to be on the template itself. So the template can have a class of "lottotemplate" that we will use, and the container can have a class of "lottocontainer".

    Moving away from unique identifiers helps us to make the code more flexible so that it can be used in multiple places on the same page, if desired.
    We can help to keep the code simple too by using document.querySelector(), which works well down to IE8. We can achieve IE7 and some earlier by using a querySelector polyfill, one of which can be found in http://www.calormen.com/polyfill/polyfill.js

    HTML Code:
    <div class="lottocontainer">...</div>
    <div class="lottotemplate">...</div>
    I want to call it with something like the following:

    Code javscript:
    lotto({
        container: document.querySelector('.lottocontainer'),
        template: document.querySelector('.lottotemplate')
    });

    which means making lotto a global object. And hopefully that will be the only global object that the code has.

    So take the existing lotto function:

    Code:
    var lotto = function (options) {
        ...
    },
        form = ...;
    and turn it in to a global object

    Code javascript:
    window.lotto = function (options) {
        ...
    };
     
    var form = ...;

    We can't call lotto yet though with our desired container and template information. Not before some cleaning up has occurred so that we don't end up making lots of in-code calls to, for example, opts.container

    What we need to do is to move the remaining code within the lotto function to their own functions, so that parameterised calls can be easily made to them.
    Before we do that though, we should remove the direct assignment of an element to the container and template variables, and just use a classname selector there for them instead.

    We currently have:

    Code:
    container = document.querySelector('.container'),
    template = document.getElementById('template').cloneNode(true),
    playButton = template.getElementsByTagName("input")[0],
    resultDiv = template.getElementsByTagName("div")[0];
    ...
    template.removeAttribute('id');
    container.innerHTML = '';
    container.appendChild(template);
    Let's replace the template with a classname selector instead. That will help us to prepare for passing that info in to the lotto function, and we can move that cloneNode to a better place too.

    HTML Code:
    <div class="lotto template">
    ...
    </div>
    Code css:
    .template {
        display:none;
    }

    Code javascript:
    templateSelector = '.lotto.template',
    ...;
    // and further down below the roll function
    var template = document.querySelector(templateSelector).cloneNode(true);

    The HTML and the CSS are a lot better there now. Because we are now using a selector to get the template, the playButton and resultDiv variables can also be replaced by selectors, with the playButton and resultDiv variables being moved down to where the template is dealt with too.

    Also, once the template has been cloned, it really isn't template content anymore. So this could be a good time too now to change things from template to content.

    Code javascript:
    container = document.querySelector('.container'),
    templateSelector = '.lotto.template',
    playSelector = 'input',
    resultSelector = 'div';
    ...
    var content = document.querySelector(templateSelector).cloneNode(true),
        playButton = content.querySelector(playSelector),
        resultDiv = content.querySelector(resultSelector);
    removeClass(content, 'template');
    ...
    container.innerHTML = '';
    container.appendChild(content);

    Thanks to the addition of a removeClass function from http://snipplr.com/view/3561/ we now have selectors for the template and its parts, and the template classname is neatly removed from the content that goes in to the container.

    Here's what we have at this stage. http://jsfiddle.net/pmw57/8R42e/20/

    Tomorrow we should be able to use a similar selector for the container, and move the code below the roll function in to its own separate function for easy handling.
    That should help us to easily call the lotto function with our own custom selectors for the container and template, instead of needing to have them embedded within the lotto code.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  21. #21
    Gre aus'm Pott gold trophysilver trophybronze trophy
    Pullo's Avatar
    Join Date
    Jun 2007
    Location
    Germany
    Posts
    5,941
    Mentioned
    215 Post(s)
    Tagged
    12 Thread(s)
    Good stuff, Paul!

    I have two questions:

    Quote Originally Posted by paul_wilkins View Post
    I want to call it with something like the following:

    Code javscript:
    lotto({
        container: document.querySelector('.lottocontainer'),
        template: document.querySelector('.lottotemplate')
    });

    which means making lotto a global object.
    Why do we have to make lotto a global?

    Quote Originally Posted by paul_wilkins View Post
    We can't call lotto yet though with our desired container and template information. Not before some cleaning up has occurred so that we don't end up making lots of in-code calls to, for example, opts.container
    Are in-code calls really that much of a no no?
    What would be the alternative?

    Also, it's nice to see you favour concise code (such as document.querySelector()) even if it means pollyfilling the functionality for older browsers. It's always tempting not to do this and take the easy path instead (or worse still, throw jQuery at a problem).

  22. #22
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by Pullo View Post
    Why do we have to make lotto a global?
    Because that's the only effective way to use it in different types of situations on a page, or with differently named areas for the container and template.

    We might have the template as being optional, so that the container is what is used for the template.
    We might have the container auto-init itself if an appropriately named element exists.
    But to do anything with it after the page has loaded, means having it available in some manner, and that is via a globally accessible object. In much the same way that jQuery is globally accessible via the $ symbol.

    Quote Originally Posted by Pullo View Post
    Are in-code calls really that much of a no no?
    What would be the alternative?
    It's not so much of a no-no, but more of a less flexibility in how the code can be used.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  23. #23
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by paul_wilkins View Post
    Tomorrow we should be able to use a similar selector for the container, and move the code below the roll function in to its own separate function for easy handling.
    That should help us to easily call the lotto function with our own custom selectors for the container and template, instead of needing to have them embedded within the lotto code.
    We can now fix up the container side of things, which currently starts out as:

    Code:
    container = document.getElementById('container'),
    templateSelector = '.lotto.template',
    playSelector = 'input',
    resultSelector = 'div';
    ...
    var content = document.querySelector(templateSelector).cloneNode(true),
    ...
    container.innerHTML = '';
    container.appendChild(content);
    We can use the same selector technique for the container. While we are here, I'll also make the play and result selectors a bit more specific too

    HTML Code:
    <input type="button" class="play">
    <div class="result"></div>
    Code javascript:
    containerSelector = '.lotto.container',
    templateSelector = '.lotto.template',
    playSelector = '.play',
    resultSelector = '.result';
    ...
    var container = document.querySelector(containerSelector),
        content = ...

    What we want to do is to move these selectors in to the opts object, but before we do that, we should first refactor the below code in to separate functions.

    Code:
    var container = document.querySelector(containerSelector),
        content = document.querySelector(templateSelector).cloneNode(true),
        playButton = content.querySelector(playSelector),
        resultDiv = content.querySelector(resultSelector);
    
    removeClass(content, 'template');
    
    playButton.id = "play" + idx;
    playButton.value = opts.buttonText;
    playButton.onclick = function justOnce() {
        this.blur();
        if (counter > 0) {
            return false;
        }
        roll();
    };
    
    resultDiv.id = "result" + idx;
    resultDiv.innerHTML = opts.initialText;
    
    container.innerHTML = '';
    container.appendChild(content);
    What do we have there, is code to setup the content from that template, and to update the container with that content.
    That's a lot of code for just one function, due to several responsibilities being involved, so what I want to do instead is to call an init function to get things going. That init function can then pass the opts info to where it's needed.

    Code javascript:
    init(opts) {
        var container = document.querySelector(opts.containerSelector),
            template = document.querySelector(opts.containerSelector),
            content = generateContentFromTemplate(template, opts);
     
        updateContainer(container, content);
    }

    That's how I want things to be structured.

    With the generateContentFromTemplate() function we can currently move the existing code in to that function.

    Code javascript:
    function generateContentFromTemplate(template, opts) {
        var content = document.querySelector(selector).cloneNode(true),
        playButton = content.querySelector(playSelector),
        resultDiv = content.querySelector(resultSelector);
     
        removeClass(content, 'template');
     
        playButton.id = "play" + idx;
        playButton.value = opts.buttonText;
        playButton.onclick = function justOnce() {
            this.blur();
            if (counter > 0) {
                return false;
            }
            roll();
        };
     
        resultDiv.id = "result" + idx;
        resultDiv.innerHTML = opts.initialText;
     
        return content;
    }
    ...
    content = generateContentFromTemplate(templateSelector, opts);

    We will come back to this later on, after the restructuring has occurred.

    After moving that code in to the function, the roll function can't find the resultDiv variable. Because two different places want to update the resultDiv, this is a good time to create a function called updateResult just for that purpose, where we can use the selector in opts.resultSelector to specify where the update occurs.

    Because the result that we want to update may be on the page, or it may be in the template content that we are working with, we will want to pass a context to the updateResult function too. We can also default that to the document object if none is provided.

    Code javascript:
    function updateResult(html, context) {
        context = context || document;
     
        var resultSelector = opts.resultSelector,
            result = context.querySelector(resultSelector);
     
        result.innerHTML = html;
    }
    ...
    var draw = ...,
        container = document.querySelector(containerSelector);
     
    ...
    updateResult(draw.join(' '), container);

    and the following line from generateContentFromTemplate() can be replaced too:

    Code:
    resultDiv = content.querySelector(opts.resultSelector);
    ...
    resultDiv.innerHTML = opts.initialText;
    updateResult(opts.initialText, content);
    Things are now ready for us to move the selectors in to the opts object.
    We can also get the selector first from the options passed to the lotto function, before defaulting to our default selectors. That way they can be overridden when calling the lotto function.
    We do though need to protect ourself from when lotto is called with no options.

    Code javascript:
    if (!options) {
        options = {};
    }
    var opts = {
        pick: options.pick || 6,
        from: options.from || 1,
        to: options.to || 49,
        containerSelector: options.containerSelector || '.lotto.container',
        templateSelector: options.templateSelector || '.lotto.template',
        playSelector: options.playSelector || '.play',
        resultSelector: options.resultSelector || '.result',
        totalRolls: options.totalRolls || 50,
        rollDelay: options.rollDelay || 50,
        buttonText: options.buttonText || "Lotto Lucky Dip",
        initialText: options.initialText || "Your Lucky Numbers"
    },

    They are listed in an approximate order of importance, but it's entirely up to you. You can list them, for example, in alphabetical order instead of yolu like.

    The move to the opts function means that some pieces of code now need to be updated, to retrieve the information from there.
    Thanks to the restructuring though, it's not many pieces at all though.

    Code javascript:
    var result = context.querySelector(opts.resultSelector);
    ...
    container = document.querySelector(opts.containerSelector);
    ...
    playButton = content.querySelector(opts.playSelector),
    resultDiv = content.querySelector(opts.resultSelector);
    ...
    var container = document.querySelector(opts.containerSelector),
    template = document.querySelector(opts.templateSelector);

    Another change that I'll want to make is to have the defaults specified separately, and to then loop through them adding them to any missing properties from the options, but that's for later on the todo list.

    Let us now get back to that generateContentFromTemplate function. Here's the current code that we'll be working on.

    Code:
    function generateContentFromTemplate(template, opts) {
        var content = template.cloneNode(true),
            playButton = content.querySelector(opts.playSelector),
            resultDiv = content.querySelector(opts.resultSelector);
    
        removeClass(content, 'template');
        
        playButton.id = "play" + idx;
        playButton.value = opts.buttonText;
        playButton.onclick = function justOnce() {
            this.blur();
            if (counter > 0) {
                return false;
            }
            roll();
        };
    
        resultDiv.id = "result" + idx;
        updateResult(opts.initialText, content);
    
        return content;
    }
    Here's what needs to be done with it:
    • the idx parts aren't needed at all
    • the play section could do with being extracted out to its own function
    • I'm not happy with that removeClass section


    Removing the idx parts is easy, and the variable declaration further up for is can be removed too.

    Code:
    idx = document.getElementsByTagName('div').length,
    ...
    playButton.id = "play" + idx;
    resultDiv.id = "result" + idx;
    The playButton code can be moved in to its own function, and called from within the generateContentFromTemplate one:

    Code javascript:
    function setupPlayButton(button, value) {
        button.value = value;
        button.onclick = function rollLottoNumbers() {
            this.blur();
            if (counter > 0) {
                return false;
            }
            roll();
        };
    }
    ...
    setupPlayButton(playButton, opts.buttonText);

    With the removeClass part, not only does it seem like a needless detail but it also results in the following nested div HTML structure for the container:

    HTML Code:
    <div class="lotto container">
        <div class="lotto">
            <!-- template content in here -->
        </div>
    </div>
    What would be good instead, is to remove that inner div - to unwrap it from the content.

    Let's move the problem out to a separate function, so that we can clean up the generateContentFromTemplate() function and focus on this last remaining problem.

    Code javascript:
    function generateContentFromTemplate(template, opts) {
        var content = getTemplateContent(template),
            playButton = content.querySelector(opts.playSelector),
            resultDiv = content.querySelector(opts.resultSelector);
     
        setupPlayButton(playButton, opts.buttonText);
        updateResult(opts.initialText, content);
     
        return content;
    }

    That function is now looking a lot better than it was.

    The getTemplateContent() function starts off like this:

    Code:
    function getTemplateContent(template) {
        var content = template.cloneNode(true);
        removeClass(content, 'template');
        return content;
    }
    What I want to do here is to create a document fragment, and copy the child nodes of the template over to there instead.

    Code javascript:
    function getTemplateContent(template) {
        var copyOfTemplate = template.cloneNode(true),
            content = document.createDocumentFragment();
     
        while (copyOfTemplate.hasChildNodes()) {
            content.appendChild(copyOfTemplate.firstChild);
        }
     
        return content;
    }

    And now the class-handling functions, such as removeClass that we had earlier, can now be removed.

    Lastly for now, the form-handling code should be moved out of the wrapper for the lotto code, and placed at the end below the wrapper. This allows the lotto code to be easily moved out to a separate javascript file.

    So now the lotto code can be started with just a call to lotto(), or other options can be given to it to change how it behaves.

    JSLint is still happy with all of the code, when we tell it to assume that we're using a browser, and there isn't much more with the refactoring left to be done.
    The code that we now have is found at http://jsfiddle.net/pmw57/8R42e/22/

    I hope you can agree that this refactoring has been worth it.

    Off Topic:

    Mad props to the SitePoint developers.

    I foolishly performed a web browser update while the preview for this post was left in an open tab, and I was able to retrieve ALL of this super-long post using the auto-restore content link.

    It seriously helps to alleviate a bad situation for me.
    Last edited by paul_wilkins; May 4, 2013 at 14:00. Reason: adjust a double "we can now" situation
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  24. #24
    Gre aus'm Pott gold trophysilver trophybronze trophy
    Pullo's Avatar
    Join Date
    Jun 2007
    Location
    Germany
    Posts
    5,941
    Mentioned
    215 Post(s)
    Tagged
    12 Thread(s)
    So, I just finished working through all of that (Phew!)
    This has really been a good learning experience in making code more readable and therefore by definition easier to maintain.
    I hadn't expected such an in-depth response when I posted my original question.
    Thank you!

    There was one last thing I wanted to ask: if we want multiple containers, how would we differentiate between them?

    Like this?

    HTML Code:
    <div class="lotto container"></div>
    <div class="lotto container2"></div>
    <div class="lotto container1"></div>
    
    etc...

  25. #25
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,716
    Mentioned
    103 Post(s)
    Tagged
    4 Thread(s)
    Quote Originally Posted by Pullo View Post
    So, I just finished working through all of that (Phew!)
    This has really been a good learning experience in making code more readable and therefore by definition easier to maintain.
    I hadn't expected such an in-depth response when I posted my original question.
    Thank you!

    There was one last thing I wanted to ask: if we want multiple containers, how would we differentiate between them?

    Like this?

    HTML Code:
    <div class="lotto container"></div>
    <div class="lotto container2"></div>
    <div class="lotto container1"></div>
    
    etc...
    No, adding numbers to the class name is a bad way to do that.

    The next stage of things that I'll take with the code, is how to make it work in multiple situations. Even if they seem to be identical.

    I'll get on to that tomorrow.
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •