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

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

3 голосов
/ 19 ноября 2009

Я соединил эту функцию 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'});
    }
});

Ответы [ 7 ]

12 голосов
/ 19 ноября 2009

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

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

3 голосов
/ 19 ноября 2009

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

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

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

например:. 

// 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;');
}
2 голосов
/ 19 ноября 2009
  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;");
}
1 голос
/ 19 ноября 2009

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

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

Это должно хоть немного помочь.

1 голос
/ 19 ноября 2009

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

$($(this))

в пользу 

$(this)
0 голосов
/ 19 ноября 2009

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

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