Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have the following code:

1) the html part consist of two container. Both containers have a list of hour and day weeks
2) a javascript code which is used to manage these lists of inputs.

I wrote the following javascript code, using jquery and underscore (1).

The js code works but I am wondering if it is possible to improve it, because the js code consists of around about 230 lines of codes.

Maybe too much considering I am using jquery and underscore library.
Any hints to improve/clean/slim the code (1).

Here is the demo code, show.

/*global jQuery, window */
(function ($, _, window) {
    'use strict';

    var $body = $('body');
    var translator = window.ExposeTranslation;
    var copyValues,
        hasSameValues,
        checkInputs,
        createToggleButton;

    var selector = {
        updateContest: $('.update-contest'),
        reminderContest: $('.reminder-contest'),
        periodicityDaysClassName: '.data-periodicity-days',
        periodicityHoursClassName: '.data-periodicity-hours',
        btnToggleReminderClassName: '.btn-toggle-reminders'
    };

    var DAYS_MAP = {
        daily: {0: true, 1: true, 2: true, 3: true, 4: true, 5: true, 6: true}, // daily
        weekday: {0: true, 1: true, 2: true, 3: true, 4: true, 5: false, 6: false}, // weekdays
        every_mon_wed_fri: {0: true, 1: false, 2: true, 3: false, 4: true, 5: false, 6: false}, // every_mon_wed_fri
        every_tue_thu: {0: false, 1: true, 2: false, 3: true, 4: false, 5: false, 6: false} // every_tue_thu
    };

    var HOURS_RANGE = _.extend({}, _.map(_.range(24), function (i) {
        return (i === 18);
    }));

    var createSelectElement = function (items, parentElement, callback) {
        var selectElement = $('<select>'),
            optionElement;

        $.each(items, function (key, value) {
            optionElement = $('<option>').val(key).text(value);
            selectElement.append(optionElement);
        });

        parentElement.prepend(selectElement);
        if (callback) {
            selectElement
                .on('change', callback)
                .on('change', function () {
                    // it sets the same value in the select of Reminder section
                    if ($(selector.btnToggleReminderClassName).hasClass('active')) {
                        $(selector.periodicityDaysClassName + ' select').val($(this).val());
                        copyValues();
                    }
                });
        }
    };

    var selectDay = function () {
        var $this = $(this),
            labels = $(selector.periodicityDaysClassName).find('label'),
            inputs = labels.find('input'),
            value = $this.val(),
            setProp;

        setProp = function (dayRange) {
            $.each(dayRange, function (i, val) {
                $(inputs[i]).prop("checked", val);
            });
            labels.hide();
        };

        if (value === 'daily') {
            labels.hide();
            inputs.prop('checked', true);
        } else if (value === 'weekday') {
            setProp(DAYS_MAP[value]);
        } else if (value === 'weekly') {
            labels.show();
            labels.find('input').prop('checked', false);
            $(inputs[(new Date()).getDay()]).prop('checked', true);
        } else if (value === 'every_mon_wed_fri') {
            setProp(DAYS_MAP[value]);
        } else if (value === 'every_tue_thu') {
            setProp(DAYS_MAP[value]);
        }
    };

    var selectHours = function () {
        var value = $(this).val(),
            labels = $(selector.periodicityHoursClassName).find('label'),
            inputs = labels.find('input');

        if (value === 'customize') {
            labels.show();
        } else {
            $(inputs).prop('checked', false);
            $(inputs[value]).prop('checked', true);
            labels.hide();
        }
    };

    checkInputs = function () {
        // it set the select on the right place on load document
        var setSelectDayValue = function (contest) {
            var periodicityDaysInputs = contest.find(selector.periodicityDaysClassName + ' input'),
                currentDaysRange = {};

            $.each(periodicityDaysInputs, function (i, v) {
                currentDaysRange[i] = $(v).prop('checked');
            });
            // it sets the default value of days select to weekly
            contest.find(selector.periodicityDaysClassName + ' select').val('weekly');
            $.each(DAYS_MAP, function (index, dayRange) {
                if (_.isEqual(dayRange, currentDaysRange)) {
                    contest.find(selector.periodicityDaysClassName + ' select').val(index);
                    contest.find(selector.periodicityDaysClassName + ' label').hide();
                    return 0;
                }
            });
        };

        var setSelectHourValue = function (contest) {
            var periodicityHoursInputs = contest.find(selector.periodicityHoursClassName + ' input'),
                currentHoursRange = {};

            $.each(periodicityHoursInputs, function (i, v) {
                currentHoursRange[i] = $(v).prop('checked');
            });
            // it sets the default value of 'hour select element' to customize
            contest.find(selector.periodicityHoursClassName + ' select').val('customize');
            if (_.isEqual(currentHoursRange, HOURS_RANGE)) {
                contest.find(selector.periodicityHoursClassName + ' select').val(18);
                contest.find(selector.periodicityHoursClassName + ' label').hide();
            }
        };

        setSelectDayValue(selector.updateContest);
        setSelectDayValue(selector.reminderContest);
        setSelectHourValue(selector.updateContest);
        setSelectHourValue(selector.reminderContest);

        if (hasSameValues()) {
            $(selector.btnToggleReminderClassName).click();
        }
    };

    createToggleButton = function (parentElement) {
        var buttonElement = $('<button>'),
            isVisible;

        buttonElement.text('copy update periodicity');
        buttonElement.attr({
            'class': 'btn ' + selector.btnToggleReminderClassName.substring(1),
            'type': 'button',
            'data-toggle': 'button',
            'data-complete-text': 'customize'
        });
        parentElement.prepend(buttonElement);

        buttonElement.on('click', function (event) {
            // TODO it works but it is triggered two times, I should have a container of these two elements
            $(event.currentTarget).parent('.reminder-contest').find('.control-group').toggle(function () {
                isVisible = $(this).is(':visible');
                buttonElement.button(isVisible ? 'reset' : 'complete');
                if (!isVisible) {
                    selector.reminderContest.find('.form-help-block').show();
                } else {
                    selector.reminderContest.find('.form-help-block').hide();
                }
            });
        });
    };

    copyValues = function () {
        var container1 = selector.updateContest.find('input:checkbox');
        var container2 = selector.reminderContest.find('input:checkbox');
        var updateDaysSelect = selector.updateContest.find('select');
        var reminderDaysSelect = selector.reminderContest.find('select');

        container2.each(function (i) {
            $(this).prop('checked', $(container1[i]).prop('checked'));
        });
        reminderDaysSelect.val(updateDaysSelect.val());
    };

    hasSameValues = function () {
        var result = true,
            updateInputs = selector.updateContest.find('input:checkbox'),
            reminderInputs = selector.reminderContest.find('input:checkbox');

        reminderInputs.each(function (i) {
            if ($(this).prop('checked') !== $(updateInputs[i]).prop('checked')) {
                result = false;
                return false;
            }
        });
        return result;
    };

    $(function () {
        var ITEM_DAYS = {
            daily: 'daily',
            weekday: 'weekday',
            weekly: 'weekly',
            every_mon_wed_fri: 'every_mon_wed_fri',
            every_tue_thu: 'every_tue_thu'
        };
        var ITEMS_HOURS = {
            18: 'at 18 pm',
            customize: 'customize'
        };

        // it creates select for Days and Hours labels
        $.each($(selector.periodicityHoursClassName), function (i, element) {
            createSelectElement(ITEMS_HOURS, $(element), selectHours);
        });
        $.each($(selector.periodicityDaysClassName), function (i, element) {
            createSelectElement(ITEM_DAYS, $(element), selectDay);
        });

        // it creates the button for copy the "Update Periodicity" into "Reminder Periodicity"
        createToggleButton(selector.reminderContest);

        // it checks the initial values in order to set properly the select and button elements values
        checkInputs();

        $body.on('click', selector.updateContest.find('input:checkbox'), function () {
            var copyUpdatePeriodicityBtn = $(selector.btnToggleReminderClassName);
            if (copyUpdatePeriodicityBtn.hasClass('active')) {
                copyValues();
            }
        });

        selector.updateContest.closest('.control-group').addClass('periodicity-fields-container');
        selector.reminderContest.closest('.control-group').addClass('periodicity-fields-container');
        selector.reminderContest.prepend('<div class="form-help-block">'+'messages:contest.help.customize_periodicity' + '</div>');

    });

}(jQuery, window._, window));

share|improve this question

1 Answer

Considering the complexity of your gui I'd say your code is already quite trim. If you wish to cut it down I'd go about simplifying the user interface, personally I found it confusing - but then again I am using it out of context.

Whilst it's great from a code view point to reuse interfaces (i.e the duplication between update and reminder sections) - from a user pov this can be confusing when the repeatition appears constantly.

If it were me I'd take your generalised code for handling your date, repeat and time fields and abstract it away behind your own form widget. Something like:

                                        +------------------+
Summary text about repeat date and time | click to change  |
                                        +------------------+

With this kind of widget you could rework your interface:

Updates:
                                                  +------------------+
Repeats weekly, every tuesday, at 18:00 and 20:00 | click to change  |
                                                  +------------------+
Reminders:
                    +--------------+
/no reminders set/  | click to set |
                    +--------------+

[ ] tick to syncronise Reminders with Updates

Once you have that kind of layout, you'd just need to rework your code so that using either widget calls into existence your generalised gui for setting a date, frequency and time - you could use a popover, or a new admin screen. By tidying this code away in a reusable widget you'd probably find places where you could abstract and cut down code (i.e. not having to be specific with class names or input syncronisation).

Also, if you developed a interpretable and generalised way of describing a repeat event, like:

{repeats:'weekly',every:'tuesday','at':[18:00,20:00]}

This could be stored in a hidden field or attribute along with the widget once used. With this, syncronising the information between widgets suddenly becomes quite easy, wouldn't require specific fields to be constantly harmonised, and any harmonising work could be done in a simple function outside of all the widget code.

Doing things in the manner above would also help with extensibility, if you ever needed to add an extra type of event - or wanted to allow the user to define multiple update events. You would be able to add a new widget easily.

Anyway, apologies if this wasn't the answer you were looking for, as I said the code to me looks pretty good for what you have - it's more the approach that could improve things.

share|improve this answer
+1 thanks very much for having a look at my code. Your comments are greatly appreciated. – Lorraine Bernard Oct 14 '12 at 7:42

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.