急いでいます！

コードレビューをお願いします。

締切済

質問者：Tank-you
質問日時：2023/12/17 01:07
回答数：3件

私は実務経験がないためトレンドの正解がわかりません。
ajaxを使ったほうがいいよとか。
ソースコードレビューをお願いします。

<!DOCTYPE html>
<html lang="en">

<head>
<meta charset="utf-8" />
<link rel="icon" href="%PUBLIC_URL%/favicon.ico" />
<meta name="viewport" content="width=device-width, initial-scale=1" />
<meta name="theme-color" content="#000000" />
<meta name="description" content="Web site created using create-react-app" />
<script type="text/javascript">
//リスト要素の追加関数
function create_op() {
let sl_from = document.getElementById('csv_from');
let sl_to = document.getElementById('csv_to'); //対象のselect取得

// selectedで選択されている値の番号が取得されます
let idx = sl_from.selectedIndex;
// 値を取得
let txt = sl_from.options[idx].text;

var arr_idx = [];
var arr_val = [];
// 選択値を配列に追加
for (var i = 0; i < sl_from.length; i++) {
if (sl_from[i].selected) {
arr_idx.push(sl_from.options[i].value); //インデックス取得
arr_val.push(sl_from.options[i].text); //テキスト取得
}
}
var del = ",";
// カンマ区切りテキストを取得
txt = arr_val.join(del);
alert(txt);

//if (sl.length < arr.length) { //selectが空の時のみ関数実行
for (let num in arr_idx) { //arrの要素数を0から順にnumに入れて繰り返し処理
let op = document.createElement('option'); //optionを新しく毎回作る
op.value = arr_idx[num];
op.text = arr_val[num]; //optionのテキストに配列[num番目]の値を入れる
sl_to.appendChild(op); //optionをselectに追加
}
//}
}

//リスト要素の削除関数
function delete_op() {
//対象のselect取得
let sl_to = document.getElementById('csv_to');
// 選択値を削除
let idx_end = sl_to.length- 1;
for (let i = idx_end; 0 <= i; --i) {
if (sl_to[i].selected) {
sl_to.remove(i);
}
}
}
</script>
<title>CSV 出力</title>
</head>

<body>

<style>
.a {
display: block;
_display: flex;
}

.b {
display: inline-block;
_display: inline;
}

.c {
display: inline-block;
_display: inline;
vertical-align: middle;
}

.no-scrollbar::-webkit-scrollbar {
display: none;
}

.no-scrollbar::-moz-scrollbar {
display: none;
}

.no-scrollbar::-o-scrollbar {
display: none;
}

.no-scrollbar::-google-ms-scrollbar {
display: none;
}

.no-scrollbar::-khtml-scrollbar {
display: none;
}
</style>
<center>
<div class="a">
<div class="c">
<select id="csv_from" style="width:100px" size="10" class="no-scrollbar" multiple>
<option value="1">会社コード</option>
<option value="2">会社名</option>
<option value="3">住所</option>
<option value="4">TEL</option>
<option value="5">設立年</option>
<option value="6">登録者</option>
<option value="7">登録日</option>
<option value="8">更新者</option>
<option value="9">更新日</option>
<option value="10"></option>
</select>
</div>
<div class="c">
<button class="a" onclick="create_op()">＞＞</button>
<BR>
<button class="a" onclick="delete_op()">＜＜</button>
</div>
<div class="c">
<select id="csv_to" style="width:100px" size="10" class="no-scrollbar" multiple>
<option value="11">従業員数</option>
</select>
</div>
</div>
</center>
</body>

</html>

通報する

この質問への回答は締め切られました。

質問の本文を隠す

回答 (3件)

最新から表示
回答順に表示

参考程度に

No.3

回答者： fujillin
回答日時：2023/12/17 15:17

No2です。

スクリプトのカテでしたので、スクリプトについて・・・

ご提示の機能はそのままにして、スクリプトの部分をNo2の方法で書き直してみると以下のような感じになります。
（alert表示する部分は省略しました）
HTML等はそのままで、<script>の部分を置き換えれば動作すると思います。

※ ご参考までに。

<script>
const
_E = id => document.getElementById(id),
_S = id => _E(id).querySelectorAll('option:checked'),
delete_op = () => {_S('csv_to').forEach(e => {e.remove()})},
create_op = () => {
_E('csv_to').append(
...[..._S('csv_from')].map(e => new Option(e.text, e.value))
)};
</script>

- 0
- 件

通報する

参考程度に

No.2

回答者： fujillin
回答日時：2023/12/17 12:32

こんにちは

実務経験もないし、トレンドも知りませんので、通りがかりの者のたわ言として読み流してください。

全体が何をなさりたいのかの説明が無いのでさっぱりわかりませんけれど、表示される雰囲気からは、通常なら左右のリストの項目を移動するようなUIを想像しますけれど、実際は違うようなので、ミスディレクションになっていると言えるでしょう。
「＞＞」、「＜＜」のボタン表示もこれを助長していますね。

＞私の環境では Firefox 以外で綺麗に隠れることを確認しました
「.no-scrollbar」の部分のことでしょうか？
スクロールバーを隠す意図が不明ですけれど、select要素の高さ（=size)指定があるので、
　overflow: hidden;
を指定しておけば、スクロールバーは表示されなくなります。

以下は、気がつくままランダムに・・
・centerタグはすでに非推奨になっています。

スクリプトの内容もご提示のままで良いのかわかりませんけれど・・・
・「＞＞」ボタンを押し下げると、左側のリストから選択されたものが右へ移動しますが、同じものを何度でも追加可能になっています。
そういう意図での仕様なのでしょうか？

・イベントの設定は、HTML要素の属性として記述するよりも、addEventListenerとしてバインドする方法の方が多く見かけるように思います。
（別スクリプトからイベントを設定しても、上書きされないという利点があります）

・変数宣言の var は段々使われなくなる方向にあるように思います。
　（ブロックスコープの const、let を使っていると思います）

スクリプトの記述方法としては、
・例えば、選択要素を取得するのにループでチェックしていますが、セレクタで選択取得が可能ですので、削除する方の関数を例にするなら
const
　S = id => document.querySelectorAll(id + ' option:checked'),
　delete_op = () => {S('#csv_to').forEach(e => {e.remove()})};
のような方法でも可能です。
全体的に簡略な記述にすることも可能であろうと思います。
（内容としてはループ処理をするのと同じですが、配列等を操作するのに便利なメソッドがいろいろと用意されていますので）
　
・移動する方についても、選択リストをalert表示する部分までなら、上記の関数を利用して
create_op = () => {
const txt = [...S('#csv_from')].map(e => e.text).join(',');
alert(txt);
}
のような感じでも処理が可能です。

※ 何をどうしたいのか不明なので、何が良いのかもわかりかねますが、ご参考にでもなれば・・・