柳屋

ソースコードのリファクタからよもやままで

Javascriptをリファクタしてみました

ソースレビューしてると確かに機能面としては十分に満たしているんですが 設計部分でどうしても気になるところが多々あります。

ここまで言うたら細かいかなー。嫌われるかなーとか思いながらも 今後のことも考えて、レビューしているんですが。

なので気になったことを十分に吐き出す為にブログで書いてみました。

今回は特定のanchorタグのhrefにパラメータを付与し、_blankをつける処理の jsの処理となっております。

基本的にhtmlは触れないので少しきになる箇所もあるかと思いますが そこはご了承ください。

まずは改修前のコードです

function changeLinkAttr(target) {
    var targetHref = $(target).attr('href');

    $(target).attr({
        href: targetHref + '?hogehoge',
        target: '_blank'
    })
}

$('#churchBl').find('.hallDetail a').each(function() {
    if ($(this).hasClass('js-clip')) {
        return;
    } else {
        changeLinkAttr(this);
    }    
});

changeLinkAttr('.hallBnr a');

javascriptのDOMとjQueryオブジェクト

まずはchangeLinkAttrの引数がjavascriptのdom になっているのが少々気になります。 domで渡してjQueryオブジェクトに変換してる。 渡す時点でjQueryオブジェクトで渡せば変換する箇所は不要ですよね。

まずは関数部分

-function changeLinkAttr(target) {
+function changeLinkAttr($target) { // $targetとする。ローカルルールとして変数名に$をつけることによりこの変数がjQueryオブジェクトであることとする
-    var targetHref = $(target).attr('href');
+    var targetHref = $target.attr('href');

-    $(target).attr({
+    $target.attr({
        href: targetHref + '?hogehoge',
        target: '_blank'
    });
}

そして呼び出し側

$('#churchBl').find('.hallDetail a').each(function() {
    if ($(this).hasClass('js-clip')) {
        return;
    } else {
-        changeLinkAttr(this);
+        changeLinkAttr($(this));
    }    
});

-changeLinkAttr('.hallBnr a');
+changeLinkAttr($('.hallBnr a'));

呼び出しの冗長

関数とその呼び出しの二つのブロックになってます。 ただ呼び出し側が冗長な感じです。

読み解くとアンカータグにjs-clipクラスが付与されていないものに対して操作しているので 上記前提が成り立つなら関数自体に組み込めば良いです。

呼び出し側は下記のようになります

$('#churchBl').find('.hallDetail a').each(function() {
-    if ($(this).hasClass('js-clip')) {
-        return;
-    } else {
-        changeLinkAttr(this);
-    }
+    changeLinkAttr($(this))    
});
changeLinkAttr($('.hallBnr a'));

関数側にjs-clipがあるかどうかの判定を組み込み以下のようになります

function changeLinkAttr($target) {
// js-clipを持っているanchorタグは例外処理として早期リターンする
// 早期リターンすることによりインデントが深くなるのを防ぐ
+    if($target.hasClass('js-clip')) {
+        return;
+    }

    var targetHref = $target.attr('href');

    $target.attr({
        href: targetHref + '?hogehoge',
        target: '_blank'
    });
}
function changeLinkAttr(target) {
    var targetHref = $(target).attr('href');

    $(target).attr({
        href: targetHref + '?hogehoge',
        target: '_blank'
    })
}

$('#churchBl').find('.hallDetail a').each(function() {
    if ($(this).hasClass('js-clip')) {
        return;
    } else {
        changeLinkAttr(this);
    }    
});

changeLinkAttr('.hallBnr a');

呼び出し方法の統一

一旦これである程度形にはなったかなと思うので現状の形を

function changeLinkAttr($target) {
    if ($target.hasClass('js-clip')) {
        return;
    }

    var targetHref = $target.attr('href');

    $target.attr({
        href: targetHref + '?hogehoge',
        target: '_blank'
    });
}

$('#churchBl').find('.hallDetail a').each(function() {
    changeLinkAttr($(this));
});

changeLinkAttr($('.hallBnr a'));

これはもしかしたら好みにもなるかもしれませんが、関数呼び出し元がeachを使っている、使っていないでバラバラなのがすごく気になります。 特定のクラス以下のanchorタグに対しての処理なのであればeachで回すのを統一してやれば良いのではと思います。 今後また処理対象のクラスが増えた場合eachを書くことになり書く行数も増えてくるので。   eachで回す処理を関数側に組み込むと以下のようになります

-function changeLinkAttr($target) {
+function changeLinkAttr($targets) {// 対象が複数なので複数系とする
+    $targets.each(function() {
+        var $target = $(this);

        if ($target.hasClass('js-clip')) {
-            return;
+            return true;
        }

        var targetHref = $target.attr('href');

        $target.attr({
            href: targetHref + '?hogehoge',
            target: '_blank'
        });
+    });
    if ($target.hasClass('js-clip')) {
        return;
    }

    var targetHref = $target.attr('href');

    $target.attr({
        href: targetHref + '?hogehoge',
        target: '_blank'
    });
}

-$('#churchBl').find('.hallDetail a').each(function() {
-    changeLinkAttr($(this));
-});
+changeLinkAttr($('#churchBl').find('.hallDetail a'));

changeLinkAttr($('.hallBnr a'));

クラス名の定数化

これで処理の部分は完成となりました。だいぶ見やすくメンテナンス性、拡張性も上がったのではないでしょうか。 あとはjs-clipがハードコートなのが気になるので定数化しましょう。 ただjs(es5)には定数はないのであくまでローカルルールでの定数ですが

+ var EXCEPTCLASS = 'js-clip';

function changeLinkAttr($targets) {
    $targets.each(function() {
        var $target = $(this);

-        if ($target.hasClass('js-clip')) {
+        if ($target.hasClass(EXCEPTCLASS)) {
            return true;
        }

        var targetHref = $target.attr('href');

        $target.attr({
            href: targetHref + '?hogehoge',
            target: '_blank'
        });
    });

    if ($target.hasClass('js-clip')) {
        return;
    }

    var targetHref = $target.attr('href');

    $target.attr({
        href: targetHref + '?hogehoge',
        target: '_blank'
    });
}

changeLinkAttr($('#churchBl').find('.hallDetail a'));
changeLinkAttr($('.hallBnr a'));

リファクタ後のコード

これでリファクタ完了しました。 改修後のコードは以下となりました。

var EXCEPTCLASS = 'js-clip';

function changeLinkAttr($targets) {
    $targets.each(function() {
        var $target = $(this);

        if ($target.hasClass(EXCEPTCLASS)) {
            return true;
        }

        var targetHref = $target.attr('href');

        $target.attr({
            href: targetHref + '?hogehoge',
            target: '_blank'
        });
    });

    if ($target.hasClass('js-clip')) {
        return;
    }

    var targetHref = $target.attr('href');

    $target.attr({
        href: targetHref + '?hogehoge',
        target: '_blank'
    });
}

changeLinkAttr($('#churchBl').find('.hallDetail a'));
changeLinkAttr($('.hallBnr a'));

今回は記述方法の統一ってのがメインになったかなと思います。 ソースコードってやはり文章なのではないかと常々思っております。 なので記述方法が変わるとそこに違和感が発生し、ソース読解がそこで止まり デバッグ作業に時間が取られてしまいます。

なので記述方法がなるべく統一していくべきでなないかなと思う次第です。

今回はここまで

;(function(document){ var pres = document.getElementsByTagName("pre") for(var i=pres.length; i--; ){  var el = makeOl(pres[i]) pres[i].appendChild(el) } function makeOl(pre){ if (pre.className.indexOf("gist") !== -1) { return } var ol = document.createElement("ol") , li = document.createElement("li") , df = document.createDocumentFragment() , br = pre.innerHTML.match(/\n/g) || 0 ol.className = "preLine" ol.setAttribute("role", "presentation") // no lang, no line-number if( pre.className && ! /lang-./.test(pre.className) ){ br.length += 1 } for(var i=br.length; i--; ){ var li2 = li.cloneNode(true) df.appendChild(li2) } ol.appendChild(df) return ol } })(document)