Мой код jQuery работает, но он очень дурацкий с точки зрения программиста?

Question

Я собрал воедино эту функцию jQuery. Его цель - рассчитать поля всех элементов img внутри div.article чтобы сбалансировать высоту изображения с базовой сеткой документа, которая составляет 20 пикселей. Чтобы соответствовать моей базовой сетке, высота каждого изображения должна быть кратна 20. Если это не так, например, высота одного изображения составляет 154 пикселя, функция добавляет поле img в 6 пикселов, так что баланс с базовой линией сетка восстановлена.

Функция работает правильно, поэтому мой реальный вопрос: так как я не программист, мне интересно, если мой код очень дрянной, хотя он работает, и если да, то как его можно улучшить?

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

Пример HTML-кода, img с подписью:

<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, img без заголовка:

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

Изменить: уточненный код на основе предложений 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

Некоторые улучшения, которые я могу порекомендовать

1.cache $(this) внутри each() в локальной переменной

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

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

    // ....

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

   // ....

   }

});

2. вместо установки attr('style'), использовать css() команда

3. Нет необходимости делать это

$($(this))

хотя он не сломает jQuery, нет необходимости передавать объект jQuery в другой объект jQuery.

4.Use $(this).height() или же $(this).outerHeight() получить высоту элемента в браузере

5. Не специфично для jQuery, но может использовать троичный условный оператор для присвоения этого значения

// 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 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. Как заметил Алекс в комментариях, я бы также удалил лишние комментарии, которые просто повторяют то, что говорит вам код. Даже если это сценарий отладки, по моему мнению, они снижают читабельность, и комментарии должны служить только для предоставления деталей, которые не присущи чтению кода.

Answer 2

Код в порядке. Вы могли бы сделать некоторые незначительные улучшения:

• Не использовать $(this) повсюду. Присвойте это чему-нибудь рано и используйте это, чтобы не расширять элемент снова и снова.
• $(this).attr('style', "margin-bottom: " + img_margin + "px;"); может быть переписан как someEl.css('margin-bottom', img_margin + 'px');

Answer 3

Вы не должны комментировать каждую строку, чтобы сказать, что происходит, код должен сообщать вам, что происходит. (если это действительно очень странное утверждение)
Комментарии должны использоваться, чтобы сказать вам, почему что-то делается.

например:

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

может быть изменено на, что гораздо более читабельно в моих глазах:

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

1. Высота изображения не должна быть взята из элемента, вместо этого используйте $(this).height, потому что это реальная высота в браузере.

В любом случае это можно переписать гораздо короче, что-то вроде

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

Способ ускорить и упростить расчет высоты будет:

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

Это должно помочь немного по крайней мере.

Answer 6

Я думаю, что вы можете бросить

$($(this))

в пользу

$(this)

Answer 7

Вот что я хотел бы сделать, объяснение в комментариях

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