My jQuery code is working, but is it very crappy from a programmer's point of view?

Question

I cobbled together this jQuery function. It's purpose is to calculate the margins of all img elements inside div.article in order to balance the image's height with the baseline grid of the document, wich is 20 px. In order to match my baseline grid, every image height should be a multiple of 20. If that's not the case, e.g. one image's height is 154 px, the function adds a 6 px margin to the img, so that the balance with the baseline grid is restored.

The function works correctly, so my actual question is: Since I'm not a programmer, I'm wondering if my code is very crappy although it's working, and if so, how could the code be improved?

The jQuery code:

$('div.article img').each(function() {

    // define line height of CSS baseline grid:
    var line_height = 20;

    // capture the height of the img:
    var img_height = $(this).attr('height');

    // divide img height by line height and round up to get the next integer:
    var img_multiply = Math.ceil(img_height / line_height);

    // calculate the img margin needed to balance the height with the baseline grid:
    var img_margin = (img_multiply * line_height) - img_height;

    // if calculated margin < 5 px:
    if (img_margin < 5) {

        // then add another 20 px to avoid too small whitespace:
        img_margin = img_margin + 20;  
    }

    // if img has caption:
    if ($($(this)).next().length) {

        // then apply margin to caption instead of img:
        $($(this)).next().attr('style', "margin-bottom: " + img_margin + "px;");
    } else {

        // apply margin to img:
        $(this).attr('style', "margin-bottom: " + img_margin + "px;");
    }
});

HTML code example, img with caption:

<div class="article">
   <!-- [...] -->
    <p class="image">
        <img src="http://example.com/images/123.jpg" width="230" height="154" alt="Image alt text goes here" />
        <small>Image Caption Goes Here</small>
    </p>
   <!-- [...] -->
</div>

HTML code example, img without caption:

<div class="article">
    <!-- [...] -->
    <p class="image">
        <img src="http://example.com/images/123.jpg" width="230" height="154" alt="Image alt text goes here" />
    </p>
   <!-- [...] -->
</div>

Edit: refined code based on Russ Cam's suggestions:

var line_height = 20;
var min_margin = 5;
$('div.article img').each(function() {
    var $this = $(this); // prefixed variable name with $ to remind it's a jQuery object
    var img_height = $this.height();
    var img_margin = ((Math.ceil(img_height / line_height)) * line_height) - img_height;
    img_margin = (img_margin < min_margin)? img_margin + line_height : img_margin;
    if ($this.next().length) {      
        $this.next().css({'margin-bottom' : img_margin + 'px'});
    } else {
        $this.css({'margin-bottom' : img_margin + 'px'});
    }
});

Answer 1

The code is fine. You could make some minor improvements:

• Don't use $(this) all over the place. Assign it to something early and use that so you don't re-extend the element over and over.
• $(this).attr('style', "margin-bottom: " + img_margin + "px;"); can be rewritten as someEl.css('margin-bottom', img_margin + 'px');

Answer 2

1. Height of the image shouldn't be taken from the element, instead use $(this).height, because that's the real height in the browser.

Anyway it could be rewritten much shorter, something like

$('div.article').each(function() {
    var img_margin = 20 - $(this).children('img:first').height() % 20;
    if(img_margin < 5) img_margin += 20;
    if($(this).children('small').length > 0)
         $(this).children('small').attr('style', "margin-bottom: " + img_margin + "px;");
    else
         $(this).children('img').attr('style', "margin-bottom: " + img_margin + "px;");
}

Answer 3

You shouldn't comment every line to say what's hapening, the code should tell you what's happening. (unless it's a really really wierd statement)
Comments should be used to tell you why something is beeing done.

e.g.:

// if img has caption:
if ($($(this)).next().length) {

    // then apply margin to caption instead of img:
    $($(this)).next().attr('style', "margin-bottom: " + img_margin + "px;");
} else {

    // apply margin to img:
    $(this).attr('style', "margin-bottom: " + img_margin + "px;");
}

could be changed to, which is much more readable in my eyes:

// if img has caption, apply margin to caption instead
if ($($(this)).next().length) {
    $(this).next().css('margin-bottom', img_margin + 'px;');
} else {
    $(this).css('margin-bottom', img_margin + 'px;');
}

Answer 4

I think you can drop the

$($(this))

in favor of

$(this)

Answer 5

A way to speed and simplify the height calculation would be:

var img_margin = 20 - ($this.height() % 20);

That should help a little at least.

Answer 6

Here is what i would do, explanation in the comments

$(function(){

// put this variable out of the loop since it is never modified
    var line_height = 20;

$('div.article img').each(function() {

    // cache $(this) so you don't have to re-access the DOM each time
    var $this = $(this);


    // capture the height of the img - use built-in height()
    var img_height = $this.height();

    // divide img height by line height and round up to get the next integer:
    var img_multiply = Math.ceil(img_height / line_height);

    // calculate the img margin needed to balance the height with the baseline grid:
    var img_margin = (img_multiply * line_height) - img_height;

    // if calculated margin < 5 px:
    if (img_margin < 5) {

        // then add another 20 px to avoid too small whitespace:
    //use ternary operator for concision
        img_margin += 20;  
    }

    // if img has caption:
    if ($this.next().length) {

        // then apply margin to caption instead of img: - use built-in css() function
        $this.next().css("margin-bottom", img_margin);
    } else {

        // apply margin to img:  - use built-in css() function
        $this.css("margin-bottom", img_margin);
    }
});
});