JavaScriptのリファクタリングについて

前提及び実現したいこと

ここに質問の内容を詳しく書いてください。
（例）
JavaScriptでシステムを作っており、やりたいことを実装できているのですが
もっとスマートにコードを記述したいと考えています。
JavaScriptの知識が浅いため、該当コードのリファクタリングなどについてお教えいただければと思います。

該当のソースコード

//編集画面読込時のイベント
$p.events.on_editor_load = function () {
    getParentData();
}

//「伝票作成」の「コース（分類E）」の値が変更されるたびに呼び出されるイベント
$(document).on('change', '#' + $p.tableName() + '_ClassE', function () {
    getParentData();
});

//「伝票作成」の「オプション（分類G）」の値が変更されるたびに呼び出されるイベント
$(document).on('change', '#' + $p.tableName() + '_ClassG', function () {
    getParentData();
});

//Bテーブルの「分類H」の値が変更されるたびに呼び出されるイベント
$(document).on('change', '#' + $p.tableName() + '_ClassH', function () {
    getParentData();
});

//「伝票作成」の「交通費（分類Q）」の値が変更されるたびに呼び出されるイベント
$(document).on('change', '#' + $p.tableName() + '_ClassQ', function () {
    getParentData();
});

function getParentData() {

    //Bテーブルの「新コース（分類R」に値がセットされている場合のみ、Aテーブルの指定したレコードの値を取得する
    if ($('#' + $p.tableName() + '_ClassR').val()) {
        $p.apiGet({
            //Bテーブルの「分類R」の値（レコードID）を指定
            'id': $p.getControl('ClassR').val(),
            'done': function (data) {
                //Bテーブルの「数値A」にAテーブルから取得した「数値A」の値をセット
                $p.set($p.getControl('NumA'), data.Response.Data[0].NumA);
            },
            'fail': function (data) {
                console.log(data);
            }
      });
  }

    //Bテーブルの「分類G」に値がセットされている場合のみ、Aテーブルの指定したレコードの値を取得する
        if ($('#' + $p.tableName() + '_ClassG').val()) {
            $p.apiGet({
                //Bテーブルの「分類G」の値（レコードID）を指定
                'id': $p.getControl('ClassG').val(),
                'done': function (data) {
                    //Bテーブルの「数値E」にAテーブルから取得した「数値C」の値をセット
                    $p.set($p.getControl('NumE'), data.Response.Data[0].NumC);
                },
                'fail': function (data) {
                    console.log(data);
                }
        });
    }

    //Bテーブルの「分類H」に値がセットされている場合のみ、Aテーブルの指定したレコードの値を取得する
        if ($('#' + $p.tableName() + '_ClassH').val()) {
            $p.apiGet({
                //Bテーブルの「分類H」の値（レコードID）を指定
                'id': $p.getControl('ClassH').val(),
                'done': function (data) {
                    //Bテーブルの「数値D」にAテーブルから取得した「数値C」の値をセット
                    $p.set($p.getControl('NumD'), data.Response.Data[0].NumC);
                },
                'fail': function (data) {
                    console.log(data);
                }
        });
    }

    //Bテーブルの「分類Q」に値がセットされている場合のみ、Aテーブルの指定したレコードの値を取得する
        if ($('#' + $p.tableName() + '_ClassQ').val()) {
            $p.apiGet({
                //Bテーブルの「分類Q」の値（レコードID）を指定
                'id': $p.getControl('ClassQ').val(),
                'done': function (data) {
                    //Bテーブルの「数値F」にAテーブルから取得した「数値C」の値をセット
                    $p.set($p.getControl('NumF'), data.Response.Data[0].NumC);
                },
                'fail': function (data) {
                    console.log(data);
                }
        });

  }
}

yambejp

2022/07/13 00:57 編集

見た感じjQueryっぽいですが、jsにしたいのですか？それともjQueryとしてコードを整理したいのですか？ $pとか定義がないですが、なにかのライブラリを使用していますか？

m.ts10806

2022/07/13 01:09

質問じゃなく作業依頼になっていると思います。取り組んだ結果で起きている問題について質問してください。

Sento__aa

2022/07/13 11:24

yambejp様コメントありがとうございます。こちらあるアプリを使っており動きだけスクリプトを打てる仕様となっています。 m.ts10806様コメントありがとうございます。おっしゃる通り、質問ではないこと理解しております。大変申し訳ございません。

行動規範の内容に同意します

回答2件

ベストアンサー

似たようなコードのコピペが増えていってしまう原因は、セレクタに
'#' + $p.tableName() + '_ClassE'
だったりの id 指定にしていることです。
これを避けるには、共通の処理を適用したい要素には同じCSSクラスを付加して、クラスセレクタによってこれらの要素をまとめてとってきて共通処理を適用するようにします。

たとえば、

javascript
1//「伝票作成」の「コース（分類E）」の値が変更されるたびに呼び出されるイベント
2$(document).on('change', '#' + $p.tableName() + '_ClassE', function () {
3    getParentData();
4});
5
6//「伝票作成」の「オプション（分類G）」の値が変更されるたびに呼び出されるイベント
7$(document).on('change', '#' + $p.tableName() + '_ClassG', function () {
8    getParentData();
9});
10
11//Bテーブルの「分類H」の値が変更されるたびに呼び出されるイベント
12$(document).on('change', '#' + $p.tableName() + '_ClassH', function () {
13    getParentData();
14});
15
16//「伝票作成」の「交通費（分類Q）」の値が変更されるたびに呼び出されるイベント
17$(document).on('change', '#' + $p.tableName() + '_ClassQ', function () {
18    getParentData();
19});

の部分についていえば、以下の4つのidセレクタ

'#' + $p.tableName() + '_ClassE'
'#' + $p.tableName() + '_ClassG'
'#' + $p.tableName() + '_ClassH'
'#' + $p.tableName() + '_ClassQ'

に該当するHTMLの要素が仮に以下であったとします。

html
1<input id="T_ClassE" />
2<input id="T_ClassG" />
3<input id="T_ClassH" />
4<input id="T_ClassQ" />

ただし上記では $p.tableName() が返す文字列を "T" と仮定しています。これらにCSSクラス T_eghqを追加して下記のようにします。

html
1<input class="T_eghq" id="T_ClassE" />
2<input class="T_eghq" id="T_ClassG" />
3<input class="T_eghq" id="T_ClassH" />
4<input class="T_eghq" id="T_ClassQ" />

上記のようにHTMLの中で対象となる要素に class="T_eghq" を追加すれば、現状では以下のように

javascript
1$(document).on('change', '#' + $p.tableName() + '_ClassE', function () {
2    getParentData();
3});
4
5$(document).on('change', '#' + $p.tableName() + '_ClassG', function () {
6    getParentData();
7});
8
9$(document).on('change', '#' + $p.tableName() + '_ClassH', function () {
10    getParentData();
11});
12
13$(document).on('change', '#' + $p.tableName() + '_ClassQ', function () {
14    getParentData();
15});

idセレクタで個別にchangeハンドラを設定していたのが下記で済みます。（文字列の連結には + ではなくテンプレートリテラルを使うとよいでしょう）

javascript
1$(document).on('change', `.${$p.tableName()}_eghq`, function () {
2    getParentData();
3});

このように idセレクタをなるべく使わずにクラスセレクターで済ませられるようにHTML側の要素にクラスを付加していくとスクリプト側が似たようなコードのコピペになるのを防ぐことができます。

getParentData関数本体の中でも
if ($('#' + $p.tableName() + '_ClassR').val()) {
といったように、idセレクタが使われていますが、これも「値が入っていたらapiGetする要素」に対して共通のCSSクラスを付加することで、似たようなコードを4回繰り返しているのをリファクタできると思います。

投稿2022/07/13 03:31

編集2022/07/13 04:06

退会済みユーザー

総合スコア0

Sento__aa

2022/07/14 00:22

su507様ご回答いただきありがとうございます。とても丁寧にご説明いただき、またわかりやすくとても参考になりました！jsの基礎的なものを理解していないため本当に助かりましたm(__)m

行動規範の内容に同意します

同じようなコードは関数化しましょう。

js
1function foo(klass, srcColumn, destColumn) {
2 if ($(`#{$p.tableName()}_${klass}`).val()) {
3   $p.apiGet({
4     'id': $p.getControl(klass).val(),
5     'done': function (data) {
6       $p.set($p.getControl(destColumn),
7              data.Response.Data[0][srcColumn]);
8     },
9     'fail': function (data) {
10       console.log(data);
11     }
12   });
13 }
14}
15
16function getParentData() {
17 foo('ClassR', 'NumA', 'NumA');
18 foo('ClassG', 'NumC', 'NumE');
19 foo('ClassH', 'NumC', 'NumD');
20 foo('ClassQ', 'NumC', 'NumF');
21}