2
\$\begingroup\$

Is this the best way of doing this. The sample code is a simple lookup of postnr (read zipcode). I'm trying to encapsulate all the client side logic inside one "class".

  1. Is this the way of doing callbacks? The this/that stuff sort of creeps me out a little
  2. Should the constructor just be a place where you declare variables? I have a init function call here that sets things up. But it is public and I wonder if there is some best practice around this?

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
  <title>Untitled Page</title>
  <script type="text/javascript" src="http://ajax.aspnetcdn.com/ajax/jQuery/jquery-1.7.2.min.js"></script>
  <script type="text/javascript">

    $(document).ready(function () {
        var controller = new ClientController("postnr", "poststed");
    });

    var ClientController = function (postNr, postSted) {
        this.postNrCtrl = $("#" + postNr);
        this.postStedCtrl = $("#" + postSted);

        this.init();
    };

    ClientController.prototype = function () {
        // private variables/methods
        var init = function () {
            // Keep reference to this
            var that = this;

            // Setup event handling
            this.postNrCtrl.change(function () {
                that.getPostSted($(this).val());
            });
        };

        var getPostSted = function (postnr) {
            // keep reference context/this
            var that = this;

            $.getJSON("http://fraktguide.bring.no/fraktguide/api/postalCode.json?country=no&pnr=" + postnr + "&callback=?", function (data) {
                that.postStedCtrl.val(data.result);
            });

        };

        // public variables/methods
        return {
            init: init,
            getPostSted: getPostSted
        };

    } ();

  </script>
</head>
<body>
  <label for="postnr">Postnr</label> <input type="text" id="postnr" />
  <label for="poststed">Poststed</label> <input type="text" id="poststed" />
</body>
</html>
\$\endgroup\$
1
\$\begingroup\$

Update: To answer your specific questions (which I just now realized I completely neglected):

  1. The this/that stuff is one way of doing "context resolution" (you used the word "callback", but that's when you pass a function to another function). There are other ways, such as Function.bind/jQuery.proxy, but in your case assigning a local that variable is a valid and customary way of doing it.

  2. Sure, you can have the constructor do nothing except declare and assign instance variables. But there are no written or unwritten rules regarding a construtor's responsibilities, so it's up to you what it should do. As you say the init method ends up being public, which isn't terribly useful - in fact, init is only useful once in the object's lifetime, so it should probably be in the constructor. And if it's in the constructor, you don't need to assign instance variables; they work fine as local variables and closures. And if they're closures, you don't need the this/that either...

... and so you don't actually need a "class" anymore, which is how I arrived at the code below.


It depends on your specific needs, but with the example you give, it seems like overkill to have a "class" (constructor + prototype) to handle that bit of logic. You can make do with:

$(function () { // shorthand for $(document).ready(...)
  var postNrCtrl   = $("#postnr"),
      postStedCtrl = $("#poststed");

  postNrCtrl.on("change", function () {
    var postnr = jQuery.trim(this.value);

    // if the field is empty, clear the postStedCtrl and stop
    if( !postnr ) {
      postStedCtrl.val("");
      return;
    }

    // Return if the value isn't 4 digits (i.e.
    // if it isn't a valid Norwegian postal code)
    if( !/^\d{4}$/.test(postnr) ) {
      // perhaps you want to alert the user here before returning
      return;
    }

    // Make an explicit JSONP request to get around same-origin policy
    // jQuery takes care of encoding the params and adding the callback param
    $.ajax({
      type:     "GET",
      dataType: "jsonp",
      url:      "http://fraktguide.bring.no/fraktguide/api/postalCode.json",
      data: {
        country: "no",
        pnr:     postnr
      }
    }).done(function (data) {
      if( data && data.valid ) {       // check the response a little
        postStedCtrl.val(data.result); // postStedCtrl is available via closure
      }
    });
  });
});

Here's a demo

On a more general note: For things like controller-logic, I find "classes" too cumbersome. You only need 1 instance of a controller, so coding a prototype and construtor only to instantiate it once, seems like overkill in a language like JavaScript that has object literals and closures.

\$\endgroup\$
  • \$\begingroup\$ "But there are no written or unwritten rules regarding a construtor's responsibilities" - I disagree. There is almost certainly an unwritten rule that a constructor should only ever prepare an object to be used. If there isn't, their should be - I'm having to debug code now that does lots of silly things in the constructor and it's a pain. \$\endgroup\$ – Dan Pantry Feb 11 '16 at 9:02
  • \$\begingroup\$ @DanPantry Fair point. My argument was a bit exaggerated. Of course you don't want your constructor to be a tangled mess; I was just rather presenting a counterpoint to the constructor being "just a place where you declare variables" as OP wrote. You're right that the constructor should only "prepare an object"; I just meant that that can involve more than just declaring vars. In OP's case preparation had been split into constructor and init when there was really no need. \$\endgroup\$ – Flambino Feb 11 '16 at 10:21

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.