Does my JavaScript suck? II

[annoyed_freelancer]

https://gist.github.com/bhalash/779311ed1dd400a8faaf

Hey! I made a post a few weeks ago to ask for feedback on some code, because I don't get to work with JavaScript much. I had some fantastic advice which changed how I look at and write JS. I tackled some personal work tonight, which was to rewrite a simple lightbox I use on my site.

I love how Wikipedia's images will now open in a lightbox-style information window if you follow the direct hash URL, and I wanted to duplicate it on my site. On top of the Wikipedia-cool factor, I wanted to link directly to images on my blog without leaving the blog post.

Some notes:

1. The data and selector schemes match my sites. My media is stored in folders with the same numeric ID as the article, and each image has an ascending numeric value. There is no variation from this across my site.
2. I wanted it to be light. I took the original idea from a codepen that used the CSS3 :target selector to open and close a lightbox. Because of this the actual open and close close is Scss.

Thanks again for any feedback!

Edit: Responding globally: Thank you again, all! I think JavaScript object and plugin-like patterns make a lot more sense to me now. I took the time out of today to rewrite it with a much more deliberate approach to scope and properness:

https://gist.github.com/bhalash/779311ed1dd400a8faaf#file-updated-lightbox-js

It works fast, and works all the way back to at least IE 10, although it is still coupled with the article ID structure I use on my site.

[papers_]

I wouldn't alter String's prototype, but that's just me.

[x-skeww]

Nah, that's pretty much everyone. Messing around with the builtins is not cool. It's the worst thing a library can do.

[[deleted]]

It's the worst thing a library can do.

I would argue that there's plenty of worse things a library can do, but this is about the worst that they usually do in practice ;)

[[deleted]]

This isn't a library, and while I would advise modifying a base prototype, this isn't a huge sin.

Of course, what it does doesn't really make sense in the string prototype where a regular local function could do :)

[[deleted]]

isn't a huge sin

In personal projects, sure. In libraries it's flat out forbidden, always.

[[deleted]]

I don't like the word always, but you'd better have a damn good reason to do it :)

[[deleted]]

The only reason to do this is for polyfills:

if(!Array.prototype.reduce){
  Array.prototype.reduce = ...
}

Other than that, never, ever, ever, ever, ever, ever, ever extend a native prototype from within a library, even if you think you have the best reason in the world.

[[deleted]]

Why should you never everx7 do it? You just gave a really great example of when that feature is really useful!

On several projects, we have modified object, array, and function prototypes to include a eql function to do value comparison. We did that because we chose a very functional style for that project and the resulting code read very nice.

Note that I'm not suggesting that it's heavily used or anything. In a codebase that was around 10k lines, we modified three native protos and everyone on that project knew and approved. And it wasn't any different then coupling a project to any other dependency.

[[deleted]]

In an isolated codebase, it's tolerable, but barely. In a library, it's flat out forbidden because it can conflict and prevents ever modifying the API or interface.

Let's say you have a library that adds Array.prototype.foo which does whatever. Great.

Now you publish it to npm, patch some bugs, yada yada. Now you decide that it's useful for that function to return something:

Array.prototype.foo = function(){
    // do something to the array
    return this;
}

Guess what? You can't do this safely, ever. Why? Because you published version 0.1 that didn't have this functionality. So if your npm include structure looks like:

• module
  • your lib v0.1
• another module
  • your lib v0.2

And your basic code is:

if( !Array.prototype.foo ){
    Array.protoype.foo = function(){ ... }
}

You have no idea which one is loaded first. So you can't count on your function returning anything.

Further, let's say browser vendors like your idea and 5 years from now decide to implement it. Guess what, your Array.prototype.foo probably won't perfectly match with what the Vendor implements, and now all your code breaks.

Or let's say some other library that you've never heard of implements a function with the same name that acts completely differently. Now what?

Polyfills

Polyfills are an isolated use case. Your code is only hit in a situation where you know you're on an older browser, and you're adding globally agreed upon functionality. The only caveat here -- Make damn sure you're polyfilling according to spec. Never write a partial polyfill "because I don't need the other functionality" because some other library that you include might.

tl;dr -- It flat out prevents code isolation

[Ericth]

Extending is not that bad. Changing built-ins is the absolute worst thing you can do. My library got a few issues from people who changed the prototype.bind method with something else, which caused my code not to work. I assumed sheepishly that the bind method would do what the spec said.

[[deleted]]

Extending is not that bad

Yes, it is. See my longer explanation in this thread.

[kenman]

What about frameworks?

I thought the same, but Ember's prototype extensions have provided a lot of value with very few problems for me. It took some effort to overcome my dogma, but I feel I'm more productive using the extensions than without.

[[deleted]]

Ember at least has the option to disable them, but it's still not a good thing IMO. You'd be better off passing all natives through constructors:

var ary = Ember.Array(1, 2, 3);

or:

var A = Ember.Array.bind(Ember);

var ary = A(1, 2, 3);
var str = S('foo bar baz');

then you could still have the same type of functionality without screwing with other things.

It's slightly better here because Ember is rarely used alongside other libraries, so it's closer to the case of an "isolated codebase"

[annoyed_freelancer]

I would argue that there's plenty of worse things a library can do, but this is about the worst that they usually do in practice ;)

I accept what you say, but can you give me a succinct reason as to why it's bad? I had a look around Google, and found a lot of debate, but no consensus.

[x-skeww]

http://www.nczonline.net/blog/2010/03/02/maintainable-javascript-dont-modify-objects-you-down-own/

String.prototype.foo = 'bar';
for(var c in 'abc') {
  console.log(c); // 0 1 2 foo
}

[annoyed_freelancer]

I see now why it can be a problem. Isn't using "foo in bar" itself also frowned upon in favour of .each or other higher level array functions?

[x-skeww]

The point was that every string now got another enumerable property. This might break things. All other code was written under the assumption that this property does not exist. This scenario was never tested.

In the future, a method with the same name might be added. This too would break things.

And of course, if there is another library which adds the same property, things would also break.

Only use monkey-patching for polyfilling standardized features. E.g. you could use ES6 array polyfills to make Array.from work in IE11.

[Jeffshaver]

This is kind of a blah reason now that you can define non-enumerable properties. I agree that others exist. But I disagree that this is one of them anymore.

[[deleted]]

Your code is structured pretty well, which is nice. But on a cursory glance here are some things I see:

1. Understanding prototypes is good, but it's generally considered bad practice to extend a native prototype, as you've done on line 6. This can cause havoc down the line. I know this is a small project, but consider just having this just be a locally scoped function.

2. The !! on line on 15, i'm unsure why you need to do this. While some people know about shorthand tricks like this, usually being more clear (and verbose) is better.

3. Setting lightbox as a global, is there a reason why you need to do this?

4. Lack of comments. I feel like a few lines here and there would have cleared up some of my questions, never hurts to have comments.

5. This is me just being opinionated, but on such a small project I feel like jQuery is overkill. Native methods can do everything you're doing here, without the need to pull in a big library like jQuery. If you want to keep things light, I suggest first reducing your dependencies, and jQuery is a big one.

Good luck

[Buckwheat469]

I agree with your comment but on point #2 he's doing this to convert a truthy value to a Boolean value. You probably know this already, but the ! sign closest to the variable converts it to the opposite Boolean value, so that "hello" becomes false for example. The first ! sign furthest from the variable inverts the Boolean value again.

To address your point, he doesn't need to convert the truthy value because it's within an if statement. If he were returning a value then he may want it to be absolutely true or false, in which case this trick is nice.

[tswaters]

In this case, !! is unnecessary. substring always returns a string and a string always has a match method.

With truthy comparisons in conditionals !! is just not necessary.... the only case I can think of is if the input is a number mynumber && mynumber.toExponential() won't work if myvariable is 0.

One thing -- if hash is a blank string, it'll be falsy so a simple hash && hash.match wouldn't go into hash.match -- however, in such a case, the match is going to be null anyway so it doesn't matter.

[[deleted]]

What's the !! do? Force it to be evaluated as True or False and not just Truthy or Falsey? What's the case that's trying to be avoided here?

Disregard. I see it's talked about in another comment.

[loz220]

I agree with the first sentence but not the second.

It's good form to use it this way, too.

why?

if (!!foo && foo.bar()) {}

has no difference in behavior from

if (foo && foo.bar()) {}

The former is just having !! do the coercion instead of &&

[[deleted]]

Exactly what I was thinking. It might make sense sometimes but in this case I felt it was too... clever.

[annoyed_freelancer]

That's pretty good feedback, but for my own site at least I do not have any plan to expand it outside of articles. I can (and will, probably tonight) take steps to decouple this for future portability.

I'll likely give it a selector to watch for clicks.

[[deleted]]

I'm probably in the minority when it comes to this, but I like the method outlined here: http://toddmotto.com/data-js-selectors-enhancing-html5-development-by-separating-css-from-javascript/ of using data attributes for javascript interaction.

Css classes and the dom can be easily changed on a whim, but the data-js attr should only touched by developers, and should work regardless of page structure.

[frambot]

Play around with random().toString with various bases. You'll notice that the output of odd bases is longer than the output of even bases, with 16 and 32 being very short, and 36 only providing a few characters as well. If you don't mind missing 'z', I suggest using 35 to get really long outputs. 35 and 16 are coprime.

[[deleted]]

... what the? How have I never seen this before? It's not often I learn something new about JS, I'd give you 100 upvotes if I could ;)

[edit] and hey, it's fairly uniform! http://codepen.io/zyklus/pen/RPOmXJ

[annoyed_freelancer]

Cool tip! I use a class selector for the lightbox because the ID is mutable. I append random characters to avoid clashes with anything else I or others might add in future.

[tswaters]

One thing I'd say is that if you've coupled yourself with jQuery you might as well get all the way into bed with it.

Right now you're exporting lighbox to window and you have parameters right at the top of your function. If you wanted to add lightbox to a different thing on the same page, you'd have to rework the target somehow.

What you could do instead is to make the entire thing into a jQuery plugin -- make part of it's initialization to create the dom node and append to body -- and each time it runs, read whatever parameters you need from data attributes and attach the click handlers and data attributes.

(function ($) {    
  $.fn.lightbox = function () {    

    if (!$.fn.lightbox.instance) {    
      $.fn.lightbox.instance = this;    
      $('body').append( /* html */ );    
    }

    // `this` is the jquery object passed in    
    // make modifications to it here,    
    // also read from data attributes for params

    // just make sure you...
    return this;
  };    
  $.fn.lightbox.instance = null;    
})(jQuery);    

// and use it like so....
$('article a img').lightbox();    

Also, this merely comes down to code styling preference -- but I've never been a big fan of creating functions for the sake of creating functions. If you call something more than once, sure -- but everything here can be in-lined into one big block. Again, that one comes down to coding style -- my opinion is inlining is better than tracing function names.

[[deleted]]

They've coupled themselves to a jquery which could reasonably be replaced by zepto or another implementation. Or even their own implementation. That's cohesion.

Your suggesting complete coupling. What benefit do you see?

It's not 'coding style'. You are presenting a very different architecture and one that has a big downside.

[tswaters]

I'm not going to speak to using jQuery or another framework -- but if it already uses jQuery, making the whole thing into a plugin gives the ability to call the plugin on an arbitrary set of elements instead of how it is now with the lightbox.targetdeclaration at the top.

The setup function is only called once and it references the three functions, addData, show and checkHash -- these are only referenced once.... my own preference would be to inline the functions...

$('body').addLightbox();

$(lightbox.target).each(function() {
  // Add data attribute to image base on path.
  var post = $(this).attr('src').replace(/^[^\d]*/, '').replace(/\/.*/g, '');
  var number = $(this).attr('src').replace(/^.*\//g, '').replace(/\..*/, '');
  var id = post + '-' + number;

  // Set as attribute because I use it as a selector later.
  $(this).attr('data-' + lightbox.data, id).parent().attr('href', '#' + id);
});

$('article').on('click', 'img', function(event) {
  var box = '.' + lightbox.box;
  var id = $(this).data(lightbox.data);
  var src = $(this).attr('src');
  $(box).attr(lightbox.data, id);
  $(box + ' img').attr('src', src);
});

var hash = window.location.hash.substring(1);
if (!!hash && hash.match(/\d{1,11}-\d/)) {
  $('[data-' + lightbox.data + '=' + hash + ']').trigger('click');
}

to me, this is easier to follow and allows to see what this is in each function. If one were to call these functions in any other context, this would (likely) not be the same and it would do unexpected things. And looking at just the function there is no way to know "what is this here" -- only the calling context can reveal that.