javascriptの簡単なコードなのですが、非効率になってしまうのを効率的にしたいです。

Question

現在練習用のサイトを作成しておりまして、javascriptを使っているのですが非効率的で困っています。もっと効率良くかけるのではないかと考えているのですが、ちゃんと動くコードにするだけでとても時間がかかってしまいました。

実現したい事はラジオボタンでAタイプを選択した状態の場合、セレクトボックスの結果を<p id="a_type">に出力してラジオボタンでBタイプを選択した場合セレクトボックスの結果を<p id="b_type">に出力するとゆうことです。

<!--ここでTYPEを選択します-->
<form name="form1" action="">
  <input type="radio" id="radio1" name="page" required checked onchange="selected_page();">A-TYPE
  <input type="radio" id="radio2" name="page" onchange="selected_page();">B-TYPE
</form>

<!--上記のTYPEに表示したい項目を選択します-->
<form name="form2" action="">
  <select id="Select1" onchange="selected_page();">
    <option>0ページ</option>
    <option>1ページ</option>
    <option>2ページ</option>
    <option>3ページ</option>
    <option>4ページ</option>
  </select>
</form>

<!--ここに結果を表示させます-->
<p id="a_type">0ページ</p>
<p id="b_type">0ページ</p>

#b_type {
  display: none;
}

function selected_page() {
	let check1 = document.getElementById("radio1").checked;
	let check2 = document.getElementById("radio2").checked;

	if (check1 == true) {
		let sel = "a_type";
		let target = document.getElementById(sel);
		target.style.display = "block"
		document.getElementById("b_type").style.display = "none"
		let selindex = document.getElementById("Select1").selectedIndex;
		switch (selindex) {
			default:
				target.innerHTML = "0ページ";
				break;
			case 1:
				target.innerHTML = "1ページ";
				break;
			case 2:
				target.innerHTML = "2ページ";
				break;
			case 3:
				target.innerHTML = "3ページ";
				break;
			case 4:
				target.innerHTML = "4ページ";
				break;
		}
	}
	else if (check2 == true) {
		let sel = "b_type";
		let target = document.getElementById(sel);
		target.style.display = "block"
		document.getElementById("a_type").style.display = "none"
		let selindex = document.getElementById("Select1").selectedIndex;
		switch (selindex) {
			default:
				target.innerHTML = "0ページ";
				break;
			case 1:
				target.innerHTML = "1ページ";
				break;
			case 2:
				target.innerHTML = "2ページ";
				break;
			case 3:
				target.innerHTML = "3ページ";
				break;
			case 4:
				target.innerHTML = "4ページ";
				break;
		}
	}
}

javascriptですが、内容が同じswitch文を二回使ってしまっていたり色々と問題がある気がしています..

お詳しい方だったらどのような感じにするのか教えていただけないでしょうか？

Answer 1

<select></select>要素から現在の値を得るのに、.selectedIndexを介しswitchする必要はありません。.valueで直接取れます。
そのため元コードの以下の部分は…

        let selindex = document.getElementById("Select1").selectedIndex;
        switch (selindex) {
            default:
                target.innerHTML = "0ページ";
                break;
            case 1:
                target.innerHTML = "1ページ";
                break;
            case 2:
                target.innerHTML = "2ページ";
                break;
            case 3:
                target.innerHTML = "3ページ";
                break;
            case 4:
                target.innerHTML = "4ページ";
                break;
        }

以下のコードに置き換えられます。

    let page = document.getElementById("Select1").value;
    target.innerHTML = page;

次に、チェックされてたラジオボタンを得るために両者のチェック状態を取得しif文で判定してらっしゃいますが、querySelectorを使って以下のように書くと簡単に現在チェックされている要素を取れます。

    let checked = document.querySelector('[name=page]:checked');

ここまででswitchとifを消して共通化でき、大幅にコードが短くなると思いますが、好みもあるでしょうがあと二点。
一つは選択されたradioボタンに対応する表示p要素のidを、jsコード中のハードコードから、html側でdata属性（名前はdata-targetにしました）で指定するようにしました。

  <input type="radio" id="radio1" name="page" required checked onchange="selected_page();" data-target="a_type">
A-TYPE
  <input type="radio" id="radio2" name="page" required onchange="selected_page();" data-target="b_type">B-TYPE

JavaScriptからは、以下のように取り出し・使用します。

    let checked = document.querySelector('[name=page]:checked');
    let targetId = checked.dataset.target;
    let target = document.getElementById(targetId);

二点目は、２つの表示用<p></p>要素の表示非表示の切り替えを.style.displayへの値の代入で行ってらっしゃいますが、これをdisplay: none;だけ記述した非表示CSSクラス（名前はhideなど）を用意して、CSSクラスの付け外しで行うようにしました。２要素なので自己満足なのですが、要素が多くなってもｊｓコードが複雑化しにくいです。
以下のような感じです。

.hide {
  display: none;
}

<p id="a_type">0ページ</p>
<p id="b_type" class="hide">0ページ</p>

    let target = document.getElementById(targetId);
    document.querySelectorAll('#a_type,#b_type').forEach(elm => {
      elm.classList.add('hide');
    });
    target.classList.remove('hide');

考え方としては、（非表示用クラス.hideを付けることで）一旦すべて隠し、ターゲットのみ外す、という２ステップになります。（#a_type,#b_typeと要素を個別に列挙していますが、あとで要素が増えるとjsコードに追加の修正が要るため、本当は共通クラスをつけるなり、divでラップするなりして、一括指定したほうがいいです）

動作確認コードはこちらになります。

Comments

[hirokuro900]

貴重なご意見ありがとうございます！とても参考になりましたm(_ _)m

思っていたよりも大分短くできて初めてみるコードも沢山あり勉強になりました！
ありがとうございましたm(_ _)m

Answer 2

こんにちは

一例として、以下のようにするかなと思います。

まずHTMLのほうで、各ラジオボタンに value 属性を追加して、その値を a_type および b_type にします。

<input type="radio" id="radio1" name="page" value="a_type" required checked onchange="selected_page();">A-TYPE
<input type="radio" id="radio2" name="page" value="b_type" onchange="selected_page();">B-TYPE

その上で、 関数 selected_page() を以下のように修正します。

function selected_page() {  
  const targetId = document.forms[0].page.value;  
  const target = document.getElementById(targetId);
  const selindex = document.getElementById("Select1").selectedIndex;
  target.style.display = "block";
  target.innerHTML = `${selindex}ページ`;

  const otherId = `${targetId === 'a_type' ? 'b' : 'a'}_type`;
  document.getElementById(otherId).style.display = "none";
}

• 動作確認用CodePen: https://codepen.io/jun68ykt/pen/mdyWzoq

参考になれば幸いです。

追記

shinji709さんのご回答で指摘されているように

const selindex = document.getElementById("Select1").selectedIndex;

target.innerHTML = `${selindex}ページ`;

の部分は、以下の一行で済みます。

target.innerHTML = document.getElementById("Select1").value;

上記のように、代入される右辺がHTMLではない文字列である場合は、 target のtextContent プロパティに代入するように

target.textContent = document.getElementById("Select1").value;

としてもよいです。

• 動作確認用CodePen: https://codepen.io/jun68ykt/pen/yLyMryV

Answer 3

同じswitch文を二回使ってしまっていたり、色々と問題がある気がしています

問題に感じているのは、「読み返すと、長くて読みにくい。」では？

同じような処理は、

1. 関数化できないかを考えます。
2. 関数の入力、出力 の値を見直します。

例えば、関連する 要素の id 属性を与える方法だと次のようにできそうです。

const formatPage = pagenum => (pagenum<5 ? pagenum : 0) + "ページ";

const getPageName = (showId, hideId, selectedId) => {
  let target = document.getElementById( showId );
  target.style.display = "block"
  document.getElementById( hideId ).style.display = "none";

  let selindex = document.getElementById( selectedId ).selectedIndex;
  target.innerHTML = formatPage(selindex);

}