美しいプログラムを書く(業務用Webアプリケーション保守編)


2012年 07月 16日

あらすじ

あなたはとある業務用 Web アプリケーションの開発・保守を任されています。
このアプリケーションは ASP.NET を用いて作成されており、
クライアントサイドは一部 jQuery を利用してナウなヤングにバカウケの UI を実装しています。

さて、今回は

  • 商品情報の変更履歴を一覧表示する。
  • 一覧から2つのバージョンを選んで差分を表示できるようにする。

という機能を実装することになりました。
これ自体はちゃちゃっと実装し、以下のようなHTMLが生成されるようにしました:

...
<table>
    <tr>
        <th>A</th>
        <th>B</th>
        <th>変更日時</th>
        <th>変更者</th>
    </tr>
    <tr>
        <td><input type="radio" name="new_version" value="9"/></td>
        <td></td>
        <td>2012-06-18 18:47:07</td>
        <td>FGH</td>
    </tr>
    <tr>
        <td><input type="radio" name="new_version" value="8"/></td>
        <td><input type="radio" name="old_version" value="8"/></td>
        <td>2012-05-17 08:47:02</td>
        <td>DEF</td>
    </tr>
    ...
    <tr>
        <td></td>
        <td><input type="radio" name="old_version" value="1"/></td>
        <td>2012-04-16 01:44:50</td>
        <td>ABC</td>
    </tr>
</table>

<input type="submit" value="AとBの差分を表示する"/>

また、特定のバージョンでの変更点を簡単に確認できるよう、
「Aの列のラジオボタンを選ぶと同じ行より一つ下にあるBの列のラジオボタンを自動で選ぶ」
という補助を実装することになりました。

これは特に難しい作業ではないので、
クライアントサイドの改修にも慣れてもらおうと考え、
今までサーバーサイドしか触れていなかったメンバーに担当してもらうことにしました。

30分後 ──

(0) できあがったコード

$(document).ready(function () {
    // seq: シーケンス番号
    $.each(["new_version", "old_version"], function () {
        $("input[name='" + this + "']").each(function (idx, elem) {
            if (idx == 0) $(elem).attr('checked', 'checked');
            $(elem).attr('seq', idx);
        });
    });
    $("input[name='new_version']").change(function () {
        var seq = parseInt($(this).attr('seq'));
        $("input[name='old_version']").eq(seq).attr('checked', 'checked');
    });
});

これはまたリファクタリングのしがいのあるコードです。
ちょっと本腰を入れて直すことにしましょう。

(1) 独自のデータを紐付けるには $.data を使う

まず「対応するラジオボタン」を選ぶための処理が臭います。
上の行のものから順に番号を振っているのですが、
この番号を紐付けるために $.attr を使っています。
$.attr は対象の要素の属性の値を参照/設定するためのものであって、
設定するならば属性の名前や値は HTML 的に妥当なものにすべきです。

$.data という、任意の要素に任意のデータを紐付けるための API が用意されているので、
ここではそれを使うべきです。また、 $.data を使えば parseInt 等をする必要もなくなります。

$(document).ready(function () {
    // seq: シーケンス番号
    $.each(["new_version", "old_version"], function () {
        $("input[name='" + this + "']").each(function (idx, elem) {
            if (idx == 0) $(elem).attr('checked', 'checked');
            $(elem).data('seq', idx);
        });
    });
    $("input[name='new_version']").change(function () {
        $("input[name='old_version']").eq($(this).data('seq')).attr('checked', 'checked');
    });
});

(2) 一つのループ内で複数の処理を行わない

次に $.each でのループ処理ですが、

  • 各列の最初のラジオボタンに初期状態でチェックを入れておく
  • 「対応するラジオボタン」を得るために番号を紐付ける

という2つの処理が混ざっています。
これは jQuery がどうこう言う以前の問題です。
混乱を避けるために各処理を分割しておきます。

$(document).ready(function () {
    var $new_versions = $("input[name='new_version']");
    var $old_versions = $("input[name='old_version']");

    $new_versions.eq(0).add($old_versions.eq(0)).attr('checked', 'checked');

    // seq: シーケンス番号
    $.each(["new_version", "old_version"], function () {
        $("input[name='" + this + "']").each(function (idx, elem) {
            $(elem).data('seq', idx);
        });
    });
    $("input[name='new_version']").change(function () {
        $("input[name='old_version']").eq($(this).data('seq')).attr('checked', 'checked');
    });
});

(3) 添字ではなく要素そのものを使う

さて、「対応するラジオボタン」を得るための処理なのですが、どうにもまどろっこしい感じが拭えません。
$.data で任意のデータを紐付けられるのですから、
わざわざ添字を使わなくとも、
直接「対応するラジオボタン」を紐付ければ良いのです。
そうすれば「seq: シーケンス番号」という謎のコメントを書くこともなくなります。

$(document).ready(function () {
    var $new_versions = $("input[name='new_version']");
    var $old_versions = $("input[name='old_version']");

    $new_versions.eq(0).add($old_versions.eq(0)).attr('checked', 'checked');

    $new_versions.each(function (i0) {
        $(this).data('pair', $old_versions.eq(i0));
    });
    $("input[name='new_version']").change(function () {
        $(this).data('pair').attr('checked', 'checked');
    });
});

(おまけ) セレクターを動的に作らない

今回の場合は(2)から(3)になる過程でごっそり削除していまいましたが、

$("input[name='" + this + "']")
    .each(function (idx, elem) {
        ...

のように動的にセレクターを作成しているのも良くありません。
この書き方では this の値に妙な文字が混ざっていると破綻します。
例えば this の値が "'foo'" の場合、上記のコードは

$("input[name='" + this + "']")
    .each(function (idx, elem) {
        ...

と書いたものと同じになります。
$.filter で絞り込めばこのような事態には遭遇しません:

var targetName = ...;
$("input")
    .filter(function () {return $(this).attr("name") == targetName;})
    .each(function (idx, elem) {
        ...

まとめ

今回の場合は $.data を知っているかどうかで読み易いコードを書けるかどうかが変わってきます。
不慣れなライブラリーでも API リファレンスには一通り目を通しましょう。

合わせて読みたい