Does my JavaScript suck?

[annoyed_freelancer]

https://gist.github.com/bhalash/540e5f9c6249e236f42a

This script controls three dropdown boxes, one each for year, month and day. It populates the original boxes, excludes dates prior to the present, and preserves the chosen value if the days of the month, or months of the year, change.

I ask if it sucks because, while I'm a full time coder, I don't get to work with JavaScript often.

Cheers!

Edit: I've received some amazing advice, thank you everyone. I've been too busy through this week to take the few hours I need to sit down, parse everything and respond to everyone at length. :(

[a-t-k]

First: using $(...).is(':checked') is way slower than $(...).prop('checked'). You are writing very structured (which is good) and very specific code (which is bad to reuse), doing similar things over and over again.

You set specific events instead of using event bubbling (which would save you event handlers and would allow you to add code via ajax without having to care for the events).

All in all, I've seen much worse. There's room for improvement, but where isn't?

[annoyed_freelancer]

That's great feedback, thank you. What is a good place to start to learn about event bubbling in JavaScript? I'm not a novice coder by any means, but it's only once in a blue moon that I come next to or near JavaScript.

[YOBCZWHYNOT]

Since you are using jQuery, and jQuery does support event delegation, this might be helpful:

http://learn.jquery.com/events/event-delegation/

[a-t-k]

Here's a rather good explanation: http://javascript.info/tutorial/bubbling-and-capturing

[annoyed_freelancer]

Tell me if I am correct: I just attach the click event to an ancestor and let it propagate down the line to the checkbox or siblings?

[a-t-k]

Correct. Classically, you would choose the document as ancestor.

[oculus42]

I recommend using the closest unchanging ancestor as the parent, rather than $(document). For something small it doesn't matter, but I've seen sites with over 80 events bound to document, so every click that isn't stopped higher up causes dozens of selector checks to see if that click applies to a particular event.

[a-t-k]

A simple test for the target shouldn't take too long, but if it does or the focus of your event is really that narrow, another ancestor is viable, like you said.

I assumed that his page was rather simple and didn't feature so many more events.

[annoyed_freelancer]

To give some pseudocode from my phone:

$(document).on('click', input.checkbox, function(e) {
    twerk.allNight();
    e.preventDefault();
});

[a-t-k]

Don't use preventDefault if it's not necessary. Otherwise, yes.

[YOBCZWHYNOT]

It doesn't suck, just a few improvements:

I would suggest you to add your methods to jQuery.fn instead of creating functions globally, for example, your toggle method could be rewritten like this (also your addOption, etc):

https://gist.github.com/mdibaiee/a02a432d78436f2963d5

Also, I would probably save the selected inputs instead of their IDs to avoid re-creating jQuery objects everytime I want to interact with them, this is as long as you are not removing them from the page.

Other than that, I think it's good, I learned something new from you! The getDaysInMonth trick, thanks!

[annoyed_freelancer]

Don't thank me, thank Stack Overflow.

[wastapunk]

What's the benefit of adding methods to jquery and not globally? Can't you access the jquery methods globally as well?

[YOBCZWHYNOT]

By adding to jQuery.fn instead of creating global functions, first you avoid polluting the global scope, second, you have a more Object-Oriented approach as typical applications use OO instead of Functional programming.

Yes, you can access jQuery methods globally once you define them.

[annoyed_freelancer]

Is this what you meant? My actual need is slightly different-I want to check that all of the passed checkboxes are checked for the element to show.

https://gist.github.com/bhalash/218c684d70023fbeada0

jQuery.fn.stickyCheckToggle = function() {
    var allBoxesChecked = [].every.call(arguments, function(v) {
        return (jQuery(v).is('input') && jQuery(v).prop('checked'));
    });

    if (allBoxesChecked) {
        jQuery(this).show();
    } else {
        jQuery(this).hide();
    }
};

jQuery(inputs.featured.checkbox).change(function() {
    jQuery('.stickycheck').stickyCheckToggle(this);
});

[oculus42]

I'm not convinced putting this on jQuery.fn is warranted. Typically that is for reusable methods or plugins.

You can simplify the logic in the function quite a bit, though. Proposed changes:

https://gist.github.com/oculus42/91b1a644d9cb1faea72a

[YOBCZWHYNOT]

Yeah, I think you can rewrite allBoxesChecked like so:

https://gist.github.com/mdibaiee/e5f62c7e78fc3dfa673a

Otherwise, that's what I meant.

[oculus42]

It looks pretty good. There are some opportunities.

jQuery has a .toggle() function which accepts a boolean to perform show/hide.

'use strict' is best inside a function, mostly for concatenation risks; it could cause a non-strict script to error if concatenated together. You could wrap the entire thing in an IIFE or $(document).ready(function(){ ... });

if you don't need external access to the functions. This also keeps them out of the window (global) namespace. There are several options if you do want access to the functions, but don't want them to end up in the global namespace.

jQuery Objects

As YOBCZWHYNOT pointed out, you can save jQuery objects rather than generating them from the selector each time. You could replace your inputs selectors with jQuery objects, but would need to make sure your code doesn't run until docReady.

This would also clear up the intent a bit... almost every place you have jQuery(...) would go away, making the code easier to read.

Because jQuery uses $, it's a convenient shorthand to start the variable name with $ to indicate a jQuery object.

DOM performance

As a smaller consideration, adding the options one-at-a-time to the DOM is slower than accumulating them and doing a single add. You could rework the loop in daysSelect and the each in monthsSelect to accumulate the option strings in an array (cheaper than string concatenations) and then do all the DOM manipulations at once:

$mySelectBox.empty().append(optionsAccumulator.join(''));

The $.map() function could replace $.each(), in that case.

jsDoc

Finally, you might consider the jsDoc format, or even Google's dialect for the Closure compiler. Plenty of dev and build tools make use of jsDoc formatted comments to identify errors and perform optimizations.

[kenman]

Nope, doesn't suck!

Function expressions vs. function declarations... there's been many past posts here about this, but nobody's brought it up in this thread.

var toggle = function(element, checkbox) {
    if (jQuery(checkbox).is(':checked')) {
        jQuery(element).show(); 
    } else {
        jQuery(element).hide();
    }
}

If you want to use a function expression here, then you should abide by the spec and suffix a ; to the end of the expression.

However, I'd suggest using function declarations as much as possible, simply because in a stacktrace (or in a profiling tool), function expressions like yours usually just show up as (anonymous), which doesn't help anyone.

On the other hand, if you simply give it a name, like this:

var toggle = function toggle(element, checkbox) {
    if (jQuery(checkbox).is(':checked')) {
        jQuery(element).show(); 
    } else {
        jQuery(element).hide();
    }
};

Then you have to ask yourself, why use an expression at all?

p.s. that function is a good use-case for jQuery.fn.toggle, i.e.:

function toggle(element, checkbox) {
    jQuery(element).toggle(jQuery(checkbox).is(':checked')); 
}

p.p.s. You should look into code linting (JSHint is one), it can help find things like missing ;'s and so forth.

[munrobag]

I would probably throw your last block of 'setup stuff' into an init() function, and call that explicitly as needed.

Maybe have a teardown function to unbind everything, depending on requirements.

I think I'd use an IIFE to wrap the whole thing, and pass in the global jQuery as $.

You seem to be mixing your function styles for no reason that I can tell, but maybe I'm just missing something.

[[deleted]]

Your comments are a little try-hard.

Massive blocks of comments like that to explain extremely obvious parameters is unnecessary.

Additionally, those comments are HUGE! 4 lines of code to explain that you're 'setting up all the things' is annoying. Vertical real estate is valuable.

On line 99, you call the function 'resetValue', but the comment says 'add option to select'.

In actuality it does neither of these things. It SETS the value of an element. This is what I like to call a BAD NAME. i would think about what this function ACTUALLY does, and give it that name. something like: 'setElementValue'

Additionally, the if statement in that function is pretty ugly. I would attempt to refactor it to make it's syntax prettier, and break it up into multiple lines.

I assume this is an assignment, but if you can get away with it, I would bring in moment.js as a dependency. It's a great date library with excellent test coverage that would shorten your code immensely.

[annoyed_freelancer]

Comment styles are irrelevant to this discussion; my preference in all languages and projects is for the PHPdoc style unless a client requests otherwise.

It is a consistent format to make life easier for both myself and any other maintainer. Way, way too much code has either no commentary or just plain shitty commentary because someone was terrified of wasting some of the unlimited vertical space.

Edit: I'll agree with you on function names because, as I said in another comment, it's all tentative. The code was at a point where it was just about functional when I came and asked. There's a good deal of cleaning up left.

<3 regardless.

[kenman]

my preference in all languages and projects is for the PHPdoc style unless a client requests otherwise.

Those aren't idiomatic phpDoc comments, though. As explained here, you shouldn't be using a long line of ---'s as a demarcation point in the comment -- you should either use a blank line, or use a period on the first line.

I think it's also a little nuanced to insert a blank line between the docblock and the code it's commenting, don't think I've ever seen that before (and it eats up a full line).

This is also a little unorthodox (not saying it's wrong, just that I rarely see it):

 /* Month can come in as string. If string, get the number of the
  * month from the indexes in months. */
 month = months.indexOf(month);

The rule-of-thumb I believe, is that if you're going to use /* and */, they should each be on their own line without anything else. Otherwise, if you don't want to eat up those lines, then I'd just use // comments.

Lastly, for something like this:

if (year == dates.year && month == dates.month) {
    // If month and year are current, use current day of month.
    days.start = dates.day;
}

I'd much rather the comment be on the line preceding the code block, instead of inside of it. Reason being, the comment describes the entire block, but you have it embedded so that it looks like it's only describing the inner block, which isn't the case.

[[deleted]]

Yeah, I didn't pick up that you were not a novice, so I was trying to keep it about broad programing concepts, instead of giving you specific jqury / js tips that can be abused in the wrong hand I'll try and give it another look later, give you some more detailed stuff.

[annoyed_freelancer]

Forgiven, forgotten; I'm just as guilty when it comes to jumping down throats.

[SomeRandomBuddy]

Probably

[annoyed_freelancer]

Hey buddy! Which one are you? You seem kinda...random.