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

Question

I combined this jQuery function. The goal is to compute the fields of all the img elements inside the div.article , in order to balance the height of the image with the underlying document grid, which is 20 pixels. To fit my base grid, each image height must be a multiple of 20. If it is not, for example. one image height is 154 px, the function adds a 6 px marker to img, so that the balance with the base grid is restored.

The function works correctly , so my real question is :. Since I am not a programmer, I am interested in my code is very crappy, although it works, and if so, how can I improve the code?

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;"); } }); 

Sample HTML code, img with title:

 <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> 

Sample HTML code, img without a header:

 <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 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

Some improvements that I can recommend

1.cache $(this) inside each() in a local variable

 $('div.article img').each(function() { var $this = $(this); // prefixed variable name with $ // to remind it a jQuery object // .... if ($this.next().length) { // .... } }); 

2. Instead of setting attr('style') use the css() command

3.No need to do this

 $($(this)) 

until it breaks jQuery, it should not pass the jQuery object to another jQuery object.

4.Use $(this).height() or $(this).outerHeight() to get the height of the element in the browser

5. No jQuery defined, but may use a ternary conditional statement to assign this value

 // if calculated margin < 5 px: if (img_margin < 5) { // then add another 20 px to avoid too small whitespace: img_margin = img_margin + 20; } 

becomes

 // if calculated margin < 5 px then add another 20 px // to avoid too small whitespace img_margin = (img_margin < 5)? img_margin + 20 : img_margin; 

6. As Alex noted in the comments, I will also remove unnecessary comments that simply repeat what the code tells you. Even if this is a debugging script, in my opinion, they reduce readability and comments should only serve to provide details that are not an integral part of reading code.

Answer 2

The code is ok. You can make some minor improvements:

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

Answer 3

• The height of the image should not be taken from the element, use $ (this) .height instead, because this is the real height in the browser.

In any case, 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 4

You do not have to comment on each line to say what is happening, the code should tell you what is happening. (if this is really a really strange statement)
Comments should be used to tell you why something has been done.

eg:.

 // 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;"); } 

can be changed to, which is much 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 5

I think you can reset

 $($(this)) 

in favor

 $(this) 

Answer 6

The way to accelerate and simplify the calculation of height:

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

That should help a little at least.

Answer 7

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); } }); });