Avoiding inline JavaScript with dynamic content

Question

I read here that you should avoid coding JavaScript inline (see item #1).

It sounds like a great idea; however, I'm not sure how to use js to select dynamic content without setting the id in the function call inline.

For example, I have a simple blog. The comments are hidden, but can be displayed by clicking an anchor (generated dynamically) that calls a JavaScript function to toggle the comments for that specific post:

$commentsButton = "<a name='comments" . $post->id . "' href='#comments" . $post->id . "'> comments (" . count($comments) . ")</a>";

toggleComments() function:

function toggleComments(id) {
    $('#' + id + ' .commentsContainer').toggle(500);
}

How do you keep from doing this kind of thing? Is it always appropriate to avoid inline JavaScript? Or is this a case where it's necessary?

Update:

I ended up using this:

$('.commentButton').each(function() {
    $(this).click(function() {
        $(this).parents('.post').find('.commentsContainer').toggle(500);
    });
});

with this:

$commentButton = "<a class='commentButton'> comments (" . count($comments) . ")</a>";

Your code could be shortened to `$('.commentButton').click(...)`. There is no need to use `each` just to call another jQuery function. — kevin cline, Mar 08 '12 at 22:46
Very similar: http://programmers.stackexchange.com/questions/126671/is-it-considered-bad-practice-to-have-php-in-your-javascript — user16764, Mar 09 '12 at 07:33
I can't believe nobody recommended event delegation on this one. — Erik Reppen, Nov 28 '12 at 06:54

score 5 · Accepted Answer · answered Nov 10 '11 at 18:37

5

How do you keep from doing this kind of thing?

In browser based javascript, this is set to the DOM element that triggered the event in the event handler. You can take advantage of that to re-use a function for any DOM element.

function toggleComments() {
    $(this).find('.commentsContainer').toggle(500);
}

Then you don't have to emit the ID into the javascript.

answered Nov 10 '11 at 18:37

Sean McMillan

5,075
25
26

For other readers, since this example looks like JQuery, in Prototype you would use `$$('.commentsContainer')` to get a list of DOM elements – Izkata Nov 10 '11 at 21:21

score 1 · Answer 2 · answered Mar 08 '12 at 19:40

The solution you ended up with is a good one. In truth the only place you should have Javascript be generated in a server side template like you were talking about is for exporting data from the server to the browser. So for example code like this is a pretty reasonable thing to do, but not actual logic

window.countries = <?php echo json_encode($countries); ?>;

Raynos · Answer 3 · 2012-03-08T19:07:23.757

0

Inline javascript is the devil. Never use it, there is never an excuse for breaking seperations of concerns other then "I'm a bad developer".

In this case you just want to bind a click handler to all your toggle comment anchors that hides the comment container.

var as = document.getElementsByClassName("toggleComments");
[].forEach.call(as, function (a) {
  var container = a.getElementsByClassName("commentsContainer")[0];
  a.addEventListener("click", function (ev) {
    container.classList.toggle("hidden");
  });
});

Rather then dynamically creating javascript inline just attach a handler to all toggle comments links.

You will have to add a class to all your toggle comments anchors though.

Legacy platform support: Yes standards compliant code doesn't run on legacy platforms. Use the DOM shim and the ES5 shim. Problem solved.

edited Mar 08 '12 at 19:07

answered Nov 10 '11 at 15:40

Raynos

8,562
33
47

4

I'm not sure that first part ("I'm a bad developer") is a very constructive/professional way to convey your expertise. – Nov 10 '11 at 15:53
@tjb1982 More professionally/constructively You would only use inline javascript out of ignorance (i.e. simply don't know better). In your case you already read an article that tells you it's a bad practice, but you still think it's ok to use a bad practice. People who think it's ok to use a bad practice are bad developers. Don't worry I'm a bad developer all the time ;) every time I use a dirty hack. – Raynos Nov 10 '11 at 16:24
Actually, I don't think it's okay. That's why I'm asking how to avoid it and/or whether there is any context where it would be unavoidable. – Nov 10 '11 at 16:28
@tjb1982 there is no context whether it's unavoidable. – Raynos Nov 10 '11 at 16:32
loud and clear. – Nov 10 '11 at 16:34
3

Hmmm...I'd argue that you should never use code that breaks on commonly used versions of IE (unless you just want to use the excuse, "I'm a bad developer"). But that's just me. – riwalk Nov 10 '11 at 17:34
2

@Stargazer712 I would argue you should stop coding the lowest denominator and use shims to emulate standard compliance. Besides it's an example piece of code, my examples are standards compliant. If he wants to use support legacy platforms then that's his problem, not mine. – Raynos Nov 11 '11 at 00:24
1

@Raynos, one of the reasons I enjoy jQuery. Code however you want--just don't bring that attitude to a job interview. – riwalk Nov 11 '11 at 16:52
1

@Raynos: +1 for the honesty of "I'm a bad developer" that nobody else wants to accept. Too many people are thin-skinned these days... – Bryan Boettcher Mar 08 '12 at 16:01
If the method associated with a button is never going to change, having the button's HTML specify `onclick=NextButtonClick()` seems like a clearer way of communicating that than having some script, someplace, attach an event handler to it. To be sure, specifying the `onclock` in the HTML doesn't guarantee that the defined action won't get changed in the JS, but a style which specifies the things that are expected to have handlers would seem easier to work with than one where everything is dynamic. – supercat Jul 20 '14 at 19:20

Avoiding inline JavaScript with dynamic content

3 Answers3