0
\$\begingroup\$

I'm a beginner at JavaScript and I'm working on building a mini-framework just to become familiar with what good code looks like. In my application I often have the need for the user to enter in a list of things they want to send to the server for querying purposes. So currently I have this fiddle. When I user enters in something into the text box the JS will check to see if that item has already been entered and disallow it if it has, or it will add the text entered into the text box and the user can see what they're going to send to the server (that part is taken out for brevity's sake). I know this isn't quite there, but I would like to do something like $('#control').holder(optionsForHolderControl) and allow the behavior to be as dynamic as possible. While I know that I'm not quite there yet, I would like to know what the experts out there would change about my code as it stands right now (organization, is it OK to have these closures in there, do the functions need to be properties, etc).

$(document).ready(function () {
    $('#textButton').click(function () {
        var options = {
            button: '#textButton',
            textBox: '#textBox',
            catcher: '#catcher'
        };
        //fxn call
        stick($(options.textBox), options.catcher);
    });
    //this takes text entered into a text box and makes a div with a strip class
    //and puts it inside the holder div
    function stick(textBox) {
        /***********variables*****************/
        var text = $(textBox).val();
        var holderArray = [];
        //gets all of the text elements 
        $('.holder .strip').each(function () {
            holderArray.push($(this).text());
        });
        /*************functions***************/
        function addStrip() {
            var $strip = $('<div>').text(text).appendTo(catcher).addClass('strip');
        }

        function clearTextbox() {
            $(textBox).val('');
            $(textBox).focus();
        }
        //duplicates aren't allowed in the holder container 
        if ($.inArray(text, holderArray) != -1) {
            alert('that item has already been entered');
        }
        else {
            addStrip();
        }
        clearTextbox();
    };
});
\$\endgroup\$
3
\$\begingroup\$

The starting point in plugin creation is most likely the official documentation here: http://learn.jquery.com/plugins/basic-plugin-creation/

You can start from the very simple plugin that actually contain the function stick as follows:

$.fn.holder = function (options) { /* ... stick's code goes here ... */ return this; };

and this will be the base for your plugin.

Then later you can call

$('#control').holder({ ... some options ... });

And the defined method should be called.

This approach will probably change and below is the review of the code you actually posted, assuming that later you will change it anyway.

The coding style of the presented code is good. I only recommend to remove /***********variables*****************/ and /*************functions***************/ because they are usually become inconsistent with the code blocks below them. Even now the variables are mixed with expressions.

The logic here is unclear:

$('#textButton').click(function () {
    var options = {
        button: '#textButton',
        textBox: '#textBox',
        catcher: '#catcher'
    };
    stick($(options.textBox), options.catcher);
});

this in click handler is the clicked object - $(this) === $(options.textBox) so it it makes sense to move options outside of the click handler and use options.button in $('#textButton').

Also in strip it is not necessary to use $(textBox) because textBox is a jQuery object already.

It is not clear why you have added the functions addStrip and clearTextbox - they are used only once so their code can be just inlined.

$(textBox).val('');
$(textBox).focus();

The power of jQuery allows you to chain the methods (and textBox is already a jQuery object):

textBox.focus().val('');

And the main logic issue - currently the code on the halfway between pure jQuery implementation without model at all and model-driven approach (the code recreates model-like construction on every click). So it is better to select one of the ways and make a step:

  • remove holderArray and just use $(''.holder .strip'').filter(function () { return $(this).text() === text; }) instead of the array routines
  • or (and what is better) move holderArray and add code that keeps it consistent with the list. You can check my article on JavaScript MVC for more info on this approach: http://alexatnet.com/articles/model-view-controller-mvc-javascript
\$\endgroup\$
  • \$\begingroup\$ Thanks for the criticism, this is the kind of thing I was looking for. I've never used any MVC before. I've only been learning JavaScript besides kiddy jQuery stuff for a couple months so I don't know if I'm quite ready for that yet, but might as well get familiar with it. Thanks :) \$\endgroup\$ – wootscootinboogie Nov 8 '13 at 2:25
  • \$\begingroup\$ You are welcome. If you like style of my reviews you can contact me directly for further reviews because I'm not frequent guest on this site. Good luck. \$\endgroup\$ – Alex Netkachov Nov 8 '13 at 13:59

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.