Mon code jQuery fonctionne, mais est-il très moche du point de vue du programmeur?

Question

J'ai bricolé cette fonction jQuery. Son but est de calculer les marges de tous les éléments img à l'intérieur div.article afin d'équilibrer la hauteur de l'image avec la grille de base du document, qui est de 20 px. Pour correspondre à ma grille de ligne de base, chaque hauteur d'image doit être un multiple de 20. Si ce n'est pas le cas, par exemple, La hauteur d'une image est de 154 px, la fonction ajoute une marge de 6 px à l'img, de sorte que l'équilibre avec la grille de ligne de base est restauré.

La fonction fonctionne correctement, donc ma question réelle est: Puisque je ne suis pas un programmeur, je me demande si mon code est très merdique bien qu'il travaille, et si oui, comment le code pourrait être amélioré?

Le code jQuery:

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

exemple de code HTML, img avec la légende:

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

exemple de code HTML, img sans légende:

<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: Code raffiné basé sur les suggestions de Russ Cam:

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

Quelques améliorations je peux recommander

1.cache $(this) à l'intérieur du each() dans une variable locale

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

    var $this = $(this); // prefixed variable name with $ 
         // to remind it's a jQuery object 

    // .... 

    if ($this.next().length) { 

    // .... 

    } 

}); 

2. au lieu de définir attr('style'), utilisez la commande css()

3.No doivent faire

$($(this)) 

alors qu'il ne se cassera pas jQuery, il est un unneccessary de passer un objet jQuery dans un autre objet jQuery.

4.Utilisez $(this).height() ou $(this).outerHeight() pour obtenir la hauteur de l'élément dans le navigateur

5.Non jQuery spécifique, mais pourrait utiliser l'opérateur conditionnel ternaire pour attribuer cette valeur

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

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

devient

// 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.Comme Alex l'a noté dans les commentaires, je supprimerais aussi les commentaires superflus qui ne font que répéter ce que la morue e vous dit. Même si c'est le script de débogage, à mon avis, ils réduisent la lisibilité et les commentaires ne doivent servir qu'à fournir des détails qui ne sont pas inhérents à la lecture du code.

Answer 2

Le code est correct.Vous pouvez apporter quelques améliorations mineures:

• N'utilisez pas $(this) partout. Affectez-le à quelque chose tôt et utilisez-le pour ne pas rallonger l'élément encore et encore.
• $(this).attr('style', "margin-bottom: " + img_margin + "px;"); peut être réécrite comme someEl.css('margin-bottom', img_margin + 'px');

Answer 3

Je pense que vous pouvez laisser tomber le

$($(this)) 

en faveur de

$(this) 

Answer 4

1. Taille de l'image ne doit pas être prise de l'élément, utilisez plutôt $ (this) .height, car c'est la taille réelle dans le navigateur.

Quoi qu'il en soit, il pourrait être réécrite beaucoup plus courte, quelque chose comme

$('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 5

Voici ce que je ferais, l'explication dans les commentaires

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

Answer 6

Vous ne devriez pas commenter chaque ligne dire ce qui se passe, le code devrait vous dire ce qui se passe. (à moins que ce soit une déclaration vraiment vraiment bizarre)
Des commentaires devraient être utilisés pour vous dire pourquoi quelque chose est en train de se faire.

par exemple:

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

pourrait être changé pour, ce qui est beaucoup plus facile à lire dans mes yeux:

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

Une façon d'accélérer et de simplifier le calcul de la hauteur serait:

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

Cela devrait aider un peu au moins.