HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Replace radio/checkbox plugin

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
radioreplacecheckboxplugin

Problem

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('');
            n_btn.parent('.input-container').append('');
        });
    }

    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();

Solution

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('')


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('');
            $btn.parent('.input-container').append('');
        });

        //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');
        });
    });
};

Code Snippets

var native_btn = element;
native_type = native_btn.attr('type');
//should be
var native_btn = element,
    native_type = native_btn.attr('type');
function replace_default_action(element) {
        var input = element;
        group = input.attr('name');
function replace_default_action(input) {
    var group = input.attr('name'); //on another note I'd prefer groupName over group
$('body').on('change', '.styled-input', function() {
            replace_default_action($this);
        })
        .on('click', '.label-replacement', function() {
            label_action($this);
        });
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'));

Context

StackExchange Code Review Q#40631, answer score: 4

Revisions (0)

No revisions yet.