4
\$\begingroup\$

I've just created my first plugin, but I think I've written too much bloated code. Could you point me to the right direction?

$.fn.replaceme = function() {

    function add_essentials(element) {
        var native_btn = element;
        native_type = native_btn.attr('type');

        native_btn.each(function() {
            var n_btn = $(this);
            n_btn.next('label').addClass('label-replacement');
            n_btn.wrap('<div class="' + native_type + ' input-container"></div>');
            n_btn.parent('.input-container').append('<span class="' + native_type + ' input-replacement"></div>');
        });
    }

    function replace_default_action(element) {
        var input = element;
        group = input.attr('name');

        $('input[name="' + group + '"]').each(function() {

            var input_select = $(this);
            if (input_select.is(':checked')) {
                input_select.siblings('.input-replacement').addClass('selected');
            } else {
                input_select.siblings('.input-replacement').removeClass('selected');
            }
        });
    }

    function label_action(element) {
        element.prevAll('.input-container').children('.styled-input').prop('checked', 'checked').trigger('change');
    }

    function live_check() {
        $('.styled-input:checked').each(function() {
            $(this).siblings('.input-replacement').addClass('selected');
        });
    };

    return this.each(function() {

        var $this = $(this);

        add_essentials($this);
        live_check();

        $('body').on('change', '.styled-input', function() {
            replace_default_action($this);
        });

        $('body').on('click', '.label-replacement', function() {
            label_action($this);
        });
    });
}

$('.styled-input').replaceme();
\$\endgroup\$
  • \$\begingroup\$ group and native_type are globals to start. Edit, also if you'd construct a demo of what your code does on jsfiddle/bin I'd appreciate it \$\endgroup\$ – megawac Feb 1 '14 at 19:19
  • \$\begingroup\$ jsfiddle.net/UCdKL/9 Here is the fiddle, thanks for your quick response \$\endgroup\$ – John McGuinnes Feb 1 '14 at 20:14
4
\$\begingroup\$

Scanning your code I have a few recommendations to make. If you update your question with a demo I'll make a few more.

As I mentioned in comments you have a few obvious issues with globals

var native_btn = element;
native_type = native_btn.attr('type');
//should be
var native_btn = element,
    native_type = native_btn.attr('type');

And similarly for group. I don't understand why you're doing this

function replace_default_action(element) {
        var input = element;
        group = input.attr('name');

Why not:

function replace_default_action(input) {
    var group = input.attr('name'); //on another note I'd prefer groupName over group

Throughout your code you make pretty good use of chaining in places it is clear and concise however there are a few spots I would adapt

Obvious one:

$('body').on('change', '.styled-input', function() {
            replace_default_action($this);
        })
        .on('click', '.label-replacement', function() {
            label_action($this);
        });

You can write this code with toggleClass to remove some lines

    var input_select = $(this);
    if (input_select.is(':checked')) {
        input_select.siblings('.input-replacement').addClass('selected');
    } else {
        input_select.siblings('.input-replacement').removeClass('selected');
    }

    //With toggle class
    var input_select = $(this);
    input_select.siblings('.input-replacement').toggleClass('selected', input_select.is(':checked'));

Also I'm pretty sure you have a logic error here:

.append('<span class="' + native_type + ' input-replacement"></div>')

Finally in javascript, especially with jQuery code (per their style guide), we prefer camelcasing over underscores for names. I'll point you to this discussion from SO.

Update Here's how I would probably write your code and here's your updated fiddle up and running. As you'll notice I removed all your helper functions that were only called once as there was no need for them and I found them confusing to be honest. I added guiding comments to replace them. I corrected the things mentioned in this review, including function names and condensed ways of writing the original. In the future I think you would benefit from trying to write your functions as straightforward and simple as possible (it took me a while to understand your original code cause of all the helper abstractions)!

$.fn.replaceme = function() {
    'use strict';

    return this.each(function() {

        var $this = $(this);
        var native_type = $this.attr('type');

        //setup button for plugin
        $this.each(function() {
            var $btn = $(this);
            $btn.next('label').addClass('label-replacement');
            $btn.wrap('<div class="' + native_type + ' input-container">');
            $btn.parent('.input-container').append('<span class="' + native_type + ' input-replacement">');
        });

        //check if is live
        $('.styled-input:checked').each(function() {
            $(this).siblings('.input-replacement').addClass('selected');
        });

        //handlers
        $('body').on('change', '.styled-input', function() {//set default action, override if already one exists
            var group = $this.attr('name');
            var $cur = $(this);
            $('input[name="' + group + '"]').each(function() {
                $cur.siblings('.input-replacement').toggleClass('selected', $cur.is(':checked'));
            });
        })
        .on('click', '.label-replacement', function() {
            $this.prevAll('.input-container').children('.styled-input').prop('checked', 'checked').trigger('change');
        });
    });
};
\$\endgroup\$
  • \$\begingroup\$ Thanks for reviewing my code, i've learned a few new tips and tricks. Especially about organizing my code. I've got one question, what is the logic error you're pointing out? \$\endgroup\$ – John McGuinnes Feb 1 '14 at 20:26
  • \$\begingroup\$ You're opening with a span and closing with a div. Should probably just remove the closing tag $('<span class="' + native_type + ' input-replacement">'); \$\endgroup\$ – megawac Feb 1 '14 at 20:42
  • \$\begingroup\$ Wow, it's time to take a nap! Haha, i feel so stupid! \$\endgroup\$ – John McGuinnes Feb 1 '14 at 21:43
  • \$\begingroup\$ @JohnElswisk I editted my answer with a counter proposal \$\endgroup\$ – megawac Feb 3 '14 at 4:37
  • \$\begingroup\$ Also looking at it again I'm certain this loop is unnecessary $('input[name="' + group + '"]').each(function() { \$\endgroup\$ – megawac Feb 3 '14 at 4:39

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.