javascriptコードへのご意見頂きたいです

前提

こんにちは。いつもお世話になっております。
現在web開発の勉強をしている学生です。
簡単なwebページの作成を行っているのですが、自分の書いたコードをなかなかコンパクトにできず気持ち悪いです。
可能な限り冗長なコードをなくしたつもりですが、まだ改善できる点などありますでしょうか。
厳しい意見もためになりますので、よろしくお願いいたします。

実現したいこと

冗長なコードの削除

発生している問題・エラーメッセージ

特になし

該当のソースコード

JavaScript
1window.addEventListener('DOMContentLoaded', function(){
2  const Qreform = document.getElementById('Qreform');
3  Qreform.addEventListener('input', function(event){
4    if (event.target.name == "country") {
5      // プルダウンの処理
6      var show1 = document.getElementById('disp1');
7      show1.innerText = event.target.value;
8    } else if (event.target.name == "schedule") {
9      // ラジオボタンの処理
10      var show2 = document.getElementById('disp2');
11      show2.innerText = event.target.value;
12    } else if (event.target.name == "place") {
13      // チェックボックスの処理
14      let checked = document.querySelectorAll('input[name="place"]:checked');
15      let out = [];
16      checked.forEach((chk) => {
17        out.push(chk.value);
18      });
19      var show3 = document.getElementById('disp3');
20      show3.innerText = out;
21    }
22    else if (event.target.name == "budget") {
23      let text = document.querySelectorAll('input[name="budget"]');
24      let out = [];
25      text.forEach((txt) => {
26        out.push(txt.value);
27      });
28      var show4 = document.getElementById('disp4');
29      show4.innerText = out + "円";
30    }
31    else{
32    }
33  });
34});
35

html
1<!DOCTYPE html>
2<html lang="ja">
3<head>
4  <title>questionnaire</title>
5  <link rel="stylesheet" type="text/css" href="22331787_Q1.css" />
6  <script type="text/javascript" src="./22331787.js"></script>
7</head>
8<body>
9  <form id="Qreform">
10    <table>
11      <caption>研修先希望調査 </caption>
12      <tr>
13        <th> 派遣先</th>
14        <td>
15          <select name="country" >
16            <option value="台湾" />台湾
17            <option value="シンガポール" />シンガポール
18            <option value="フィリピン" />フィリピン
19          </select>
20        </td>
21        <td id="disp1"></td>
22      </tr>
23      <tr>
24        <th>日程</th>
25        <td>
26          <label>
27            <input type="radio" name="schedule" value="3月" id="r"/>3月
28          </label>
29          <label>
30            <input type="radio" name="schedule" value="8月" id="r"/>8月
31          </label>
32          <label>
33            <input type="radio" name="schedule" value="12月" id="r"/>12月
34          </label>
35        </td>
36        <td id="disp2"></td>
37      </tr>
38      <tr>
39        <th>宿泊施設</th>
40        <td>
41          <input type="checkbox" name="place" value="A-HOTEL" id="c1"/>
42          <label for="c1" class="l1">A-HOTEL</label>
43          <input type="checkbox" name="place" value="B-HOTEL" id="c2"/>
44          <label for="c2" class="l1">B-HOTEL</label>
45          <input type="checkbox" name="place" value="C-RESORT" id="c3"/>
46          <label for="c3" class="l1">C-RESORT</label>
47        </td>
48        <td id="disp3"></td>
49      </tr>
50      <tr>
51        <th>予算</th>
52        <td>
53          <input type="text" name="budget">円
54        </td>
55        <td id="disp4"></td>
56      </tr>
57      <tr>
58        <td colspan="3" id="tail">
59          <input type="submit" name="sendbutton" value="送信" id="button" />
60        </td>
61      </tr>
62    </table>
63  </form>
64
65</body>
66</html>
67

試したこと

以前の質問で教えて頂いたこと(ループ処理)などを実装。

補足情報（FW/ツールのバージョンなど）

HTML5
Atom

m.ts10806

2022/10/14 02:05

JavaScriptだけではリファクタリングにならないと思います。 HTMLも提示してください。また、HTMLもある程度改修する前提でも問題ないのでしょうか。仕様も提示されたほうが良いと思います。コードの見た目だけ直しても同じ動作をしなければリファクタリングになりません。

Keisuke_NUT

2022/10/14 02:10

m.ts10806様いつもコメント・回答ありがとうございます。感謝しております。大変失礼しました、HTMLのソースコードも追加しました。はい、HTMLの改修も問題ございません。何か気づかれた点や改善点等あれば、ご指摘よろしくお願いいたします。

forer

2022/10/14 02:44

＞自分の書いたコードをなかなかコンパクトにできず気持ち悪いです。どの辺が気持ち悪いのでしょうか？

forer

2022/10/14 02:48

配列にぶち込んでforで回しifの条件式の判定回数を減らす程度ですか？そんなの部外者から見ればどうでもいい話だとおもいますが？

forer

2022/10/14 02:49

コードを見られて馬鹿にされるなどのちんけなプライドは捨てましょうなりふりかまわないほうが、うまくいきますよ

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

回答3件

javascript
1<script>
2document.addEventListener('input',e=>{
3  const f=e.target.closest('#Qreform');
4  if(!f) return false;
5  disp1.textContent= f.querySelector('[name=country]').value;
6  disp2.textContent= f.querySelector('[name=schedule]:checked')?.value;
7  disp3.textContent= [...f.querySelectorAll('[name=place]:checked')].map(x=>x.value).join(',');
8  const v=f.querySelector('[name=budget]').value;
9  disp4.textContent= v+(/^\d+$/.test(v)?"円":"");
10});
11</script>
12 <form id="Qreform">
13    <table>
14      <caption>研修先希望調査 </caption>
15      <tr>
16        <th> 派遣先</th>
17        <td>
18          <select name="country" >
19            <option value="">-</option>
20            <option value="台湾">台湾</option>
21            <option value="シンガポール">シンガポール</option>
22            <option value="フィリピン">フィリピン</option>
23          </select>
24        </td>
25        <td id="disp1"></td>
26      </tr>
27      <tr>
28        <th>日程</th>
29        <td>
30          <label>
31            <input type="radio" name="schedule" value="3月">3月
32          </label>
33          <label>
34            <input type="radio" name="schedule" value="8月">8月
35          </label>
36          <label>
37            <input type="radio" name="schedule" value="12月">12月
38          </label>
39        </td>
40        <td id="disp2"></td>
41      </tr>
42      <tr>
43        <th>宿泊施設</th>
44        <td>
45          <label>
46            <input type="checkbox" name="place" value="A-HOTEL">A-HOTEL
47          </label>
48          <label>
49            <input type="checkbox" name="place" value="B-HOTEL">B-HOTEL
50          </label>
51          <label>
52            <input type="checkbox" name="place" value="C-RESORT">C-RESORT
53          </label>
54        </td>
55        <td id="disp3"></td>
56      </tr>
57      <tr>
58        <th>予算</th>
59        <td>
60          <input type="text" name="budget">円
61        </td>
62        <td id="disp4"></td>
63      </tr>
64      <tr>
65        <td colspan="3" id="tail">
66          <input type="submit" value="送信">
67        </td>
68      </tr>
69    </table>
70  </form>

投稿2022/10/14 03:27

編集2022/10/14 03:42

yambejp

総合スコア117680

ベストアンサー

はじめに質問の本題に対する修正の一例を回答します。その後、本題以外の部分で修正すべき点を補足します。

HTMLの表示用の要素の id 属性を以下のように修正します。

diff
1-      <td id="disp1"></td>
2+      <td id="disp-country"></td>

diff
1-      <td id="disp2"></td>
2+      <td id="disp-schedule"></td>

diff
1-      <td id="disp3"></td>
2+      <td id="disp-place"></td>

diff
1-      <td id="disp4"></td>
2+      <td id="disp-budget"></td>

このように修正しておくと、JavaScript のほうは下記でよいのではと思います。

javascript
1window.addEventListener('DOMContentLoaded', function () {
2  const Qreform = document.getElementById('Qreform');
3
4  Qreform.addEventListener('input', function ({ target: { value, name } }) {
5    document.getElementById(`disp-${name}`).innerText =
6      name === 'place'
7        ? [...document.querySelectorAll('input[name="place"]:checked')].map(e => e.value)
8        : value;
9  });
10});

補足1

他にHTMLのほうで修正すべき点として下記の3点を提案します。

name="schedule" のラジオボタンの id がどれも r になっており重複しているので、重複しないように要修正
country のオプションが <option value="台湾" />台湾 という書き方になっているが、<option value="台湾">台湾</option> に要修正
country のオプションが初期表示のときに、国がどこも選ばれていないことを示すために <option value=""></option> というオプションを一番上に追加しておくとよい

補足2

上記で挙げたコード例に対してのさらなる修正です。

(1) name 属性が place の複数のチェックボックスからチェックされているものの値の配列を得る
[...document.querySelectorAll('input[name="place"]:checked')].map(e => e.value)
は name が他のチェックボックスでも使えそうなので関数化しておきます。

javascript
1const checkedValues = name =>
2  [...document.querySelectorAll(`input[name="${name}"]:checked`)].map(e => e.value);

(2) 表示用要素に入力値をテキスト設定するところを関数化します。このとき、name属性によっては表示用に加工します。

javascript
1const displayValue = (name, value) => {
2  value = name === 'budget' ? `${(+value || 0).toLocaleString()} 円` : value;
3  document.getElementById(`disp-${name}`).innerText = value;
4}

上記の(1)(2)を反映したCodepen 👉 https://codepen.io/su507/pen/OJZddEj?editors=1010

投稿2022/10/14 03:37

編集2022/10/14 04:55

退会済みユーザー

総合スコア0

同じようなコードを何度も書くのは嫌だな、と思うのは良い意識だと思います。何度も書くとミスが入りやすいですしね。

この場合は、name プロパティ値から表示先のIDと加工関数への表を持っておくとすっきりするかもしれません。

js
1const controlsDisplayMap = {
2  place: {
3    displayId: 'disp3',
4    filter: () => [...document.querySelectorAll('input[name="place"]')].map(e => e.value)
5  }
6  budget: {
7    displayId: 'disp4',
8    filter: target => `${target.value}円`
9  },
10  // country, schedule のぶんも作っておく
11};
12
13const entry = controlsDisplayMap[event.target.name];
14if (entry) {
15  document.qeurySelecotr(`#${entry.displayId}`).textContent =
16      entry.filter(event.target);
17}