javascript - 我的 jQuery 代码正在运行，但从程序员的角度来看，它是不是很糟糕？

Question

我拼凑了这个 jQuery 函数。其目的是计算内部所有 img 元素的边距，div.article以平衡图像的高度与文档的基线网格，即 20 px。为了匹配我的基线网格，每个图像高度应该是 20 的倍数。如果不是这种情况，例如一个图像的高度是 154 px，该函数会为 img 添加 6 px 边距，以便与基线平衡网格恢复。

该功能正常工作，所以我的实际问题是：由于我不是程序员，我想知道我的代码是否非常糟糕，虽然它正在工作，如果是这样，如何改进代码？

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

score 12 · Accepted Answer

我可以推荐一些改进

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() or$(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.正如亚历克斯在评论中指出的那样，我也会删除那些只是重复代码告诉你的多余评论。即使这是调试脚本，在我看来，它们也会降低可读性，并且注释应该仅用于提供阅读代码所不具备的细节。

score 3 · Accepted Answer

代码很好。你可以做一些小的改进：

不要$(this)到处使用。尽早将其分配给某些东西并使用它，这样您就不会一遍又一遍地重新扩展元素。
$(this).attr('style', "margin-bottom: " + img_margin + "px;");可以改写为someEl.css('margin-bottom', img_margin + 'px');

score 2 · Accepted Answer

图像的高度不应取自元素，而应使用 $(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;");
}

score 2 · Accepted Answer

你不应该评论每一行来说明发生了什么，代码应该告诉你发生了什么。（除非它是一个非常奇怪的声明）
注释应该用来告诉你为什么要做某事。

例如：

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

score 1 · Accepted Answer

1

我认为你可以放弃

$($(this))

有利于

$(this)

于 2009-11-19T12:31:48.427 回答

score 1 · Accepted Answer

一种加快和简化高度计算的方法是：

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

这至少应该有一点帮助。

score 0 · Accepted Answer

这是我会做的，在评论中解释

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

javascript - 我的 jQuery 代码正在运行，但从程序员的角度来看，它是不是很糟糕？

7 回答 7

Related

Reference