【初心者向け】クソコードを書かないためのテクニック【JavaScript編】
HTML-код
- Опубликовано: 26 ноя 2024
- 【エンジニアチャンネル公式Twitter】
/ engr_channel
【小川 Twitter】
/ ogaaryo
【粟島 Twitter】
/ masayan0911
【前田 Twitter】
/ shinnosuke_324
-----------------------------------------------------------------------
プロフィール
-----------------------------------------------------------------------
■エンジニアチャンネル
✅IT業界の裏事情・オフレコトーク ✅転職・就職・キャリア戦略 ✅効率的な学習方法を中心に「エンジニアになりたい方」や「IT業界で成長したい方」に向けた情報を発信しています。
■小川亮(おがわりょう)
株式会社エンジニアチャンネル 代表取締役社長
ベンチャー企業の取締役CTOとして組織・サービス両面のリーダーとして活動後、フリーランスを経て起業。
プログラミング学習、転職・就職・キャリア戦略などの豊富な相談実績に基づき今日から使えるハウツーを発信。
■粟島正俊(あわしままさとし)
株式会社テックファクトリー 代表取締役社長
エンジニアとしてキャリアをスタート。ベンチャー企業のCTOとしてサービス開発全般を行う。
フリーランスを経て、株式会社テックファクトリーを起業。自社サービスで稼ぐプロとして、エンジニアとビジネスの観点を織り交ぜて発信。
#エンジニアチャンネル #javascript #プログラミング 
Хобби
最後のやつは「JavaScript標準の関数も使えばシンプルに書けるよね」と言う事も言える。
8行目 ~ 12行目は
return Math.max(engineerEvaluation, 0);
でOK。
userの構造体はageじゃなくて生年月日が汎用性高い
第一問:isOgawa という関数であれば、判定基準が "ogawa" 以外無いので、そもそも定数にする必要ないと思う。加えて、"ogawa"以外が引数の時ダンマリなのは少し悲しい。
第二問:biggerThan〜だと'より大きい'になるので'>='より'>'を想像してしまうから関数名を変えるべき。
第三問:...(デフォルト引数か…)
第四問:むしろネストを一つ深くする方がわかりやすい。
まず、if(isEmpty(...)){
この中でisFetchingという関数をつくりifで再分岐する。つまり、
if(isEmpty(listNode)){
if(isFetching(importData.state)){
//...
}else{
//...
}
という感じ。
４問目に関しては、その修正をした場合に若干挙動が変わってしまう点は注意が必要。
処理そのものが省略されているので何ともだが、前の判定と後の判定がそれぞれ別で通るように実装されている事に何かしらの理由があった場合にその修正はマズい。
（そういう作りがそもそもクソと言うツッコミはさておき・・・ｗ）
@@yoshito323
まあ、確かにそうですねww
1つ目の if 文のなかで importData.state とかが変わる可能性がありますね(!?)前田さんが else if の話とかしてたのでそれに引きずられた感じですね。
とはいえ、無意識的にこう書き直してしまったということは、そもそも問題のあるコードであったということですかね(＞人＜;)
こういった企画大好きです。動画長かったけど、全部見ました。
最後のコード
再代入するという明確な意義があるのであればvarよりletを使った方がいい気がしますが...
そのあとで参照を目的とした新たな変数をconstで用意してあげるのがベストな気がします。
エンタメ & 学び って最高だと思います！
2問目、bigger than だと「より大きい」なので等号入ってるのも違和感のような。
同じことは思いました。
Bigger thanという表記だと、「より大きい」という意味の方が正確なので、
return age > LEGAL_AGE ;
という意味としてとらえられてしまうのではないのかなと思いました。
ついでにどうでもいいことをいうと、年齢を対象にするのであれば、Biggerよりもolderのほうがわかりやすいかなっと。（でも結局isLegalAgeが最適だよねとはなりますがｗ）
最初の問題、isOgawaと言う関数名は「真偽値を返す関数」と認識される可能性があるので変えた方がいい。
でもいい命名が思いつかないｗ
後、NAMEと言う定数名も、関数名と関連が無いのであまりよろしくない。
（２問目の「LEGAL_AGEに変更が入ったら・・・」と同じ問題が起こる）
最後の問題、varよりconstにした方が良くないですかね？
ここじゃないどっかで「varは使わないで。letにして」みたいなこと言われましたね。
scopeは可能な限り狭く、って。
以降jsでは絶対にvarを使わないおじさんが誕生しました。
4問目は、listNodeが配列っぽい名前をしているので、
「Nullかどうか？」と「空配列かどうか？」で処理を分けた方が良いと思いました
これは面白いですね！勉強にもなりますし、普通に考えて見て楽しかったです！
そしてこういう問題思いつくのすごいなと思いました！
僕もプログラミング系の話を動画にしていますが、
例に挙げられてたあまり良くないネーミングはやっちゃいがちなので気をつけようと思いました。
個人的には変数名もLEGAL_AGEで関数名もisLegalAgeにしちゃうとイコールなのかなって思うこともあると感じました。
比較の要素を関数名に入れたいなと思いました。（more thanとかupperとかoverとか？）
賛成です。isLegalAgeだとぴったり18歳じゃないとfalseな印象なので、この年齢は成人か？と言う意味で isAdult がいいんじゃないかと思いました。
個人的にはこーかな…
①isOgawa
1. 他との競合を回避するため、定数名には最低限修飾子を付ける
2. 関数名をisで始めると真偽値判定に誤認されるので適当な名前にする
3. 分岐条件も引数に渡し、汎用化に努める
4. 定数が変わった時にコードの修正が発生するので、メッセージはテンプレートリテラルを使用する
```
const PERSON_NAME = {OGAWA: ...};
function checkName (name, target) {
if (name === target) console.log(`${name}です。`);
}
checkName('ogawa', PERSON_NAME.OGAWA);
```
②bigger
1. 年齢ではなく誕生日をデータに入れる(データ構造の話なので微妙なとこですが…)
2. 真偽値を取得するので、関数名はisで始める
3. オブジェクトは分割代入する(呼び出し側との整合も含めて)
4. 年齢ではなく、ユーザーを評価対象にする
```
const LEGAL_AGE = 18;
const user = {birthday: ..., id: ..., name: ...}
function isLegally ({age}) {
return age >= LEGAL_AGE;
}
isLegally(user);
```
③hogehoge
1. 空文字列時の挙動が変わるので、変更すべきではない
変更前
name hoge
'hoge' 'hoge'
'' 'hogehoge'
(省略) 'hogehoge'
変更後
name hoge
'hoge' 'hoge'
'' ''
(省略) 'hogehoge'
④something
1. 定数は別で宣言しておく
2. オブジェクトは分割代入する
```
const STATE = {FETCHING: ...};
function doSomething ({state}, node) {
if (isEmpty(node)) {
if (state === STATE.FETCHIG) {...}
else {...}
}
}
doSomething(importData, listNode);
```
⑤method
1. 中身については触れないので特に無し、名前だけ適当にする
⑥Evaluation
1. 除算よりも乗算の方が良いのでNEGATIVE_RATEは少数で定義する
2. 計算・取得しているのはあくまでもポイントなので、そもそも関数名を変える
3. 引数名が長いので、オブジェクトの分割代入で短縮する
4. NEGATIVE_RATEは乗算で計算する
5. 下限値を判定して再代入するよりも、適当な標準の関数を使用する
```
const NEGATIVE_RATE = 0.1;
function getEngineerPoint ({frontendSkill, backendSkill, negative}) {
let point = frontendSkill + backendSkill;
point -= negative * NEGATIVE_RATE;
return Math.max(point, 0);
}
getEngineerPoint({frontendSkill, backendSkill, negative});
```
NEGATIVE_RATEは0.1にして
(negativePoint * NEGATIVE_RATE)にしたほうが良さそう｡
悪い点は10%分だけ評価から引きましょうね~って意図だと思うので､率はかけたほうがわかりやすいのと
仮に15%分にしましょうとなった際に割り算だと実現できない｡
ついでに言えば、除算よりも乗算の方が処理的には高速。
JavaScriptの場合はユースケース的にそこまで気にする事もないかも知れないけど・・・
@@yoshito323 なるほど！
減点評価はやめて加点だけにしましょう。ができませんね。
問4
個人的には条件式自体も分けたいかなと思いました。
状態増えた時に、毎回isEmpty書く必要ないと思いますので。
その上で状態を判定するメソッドに分離した方がスッキリしそうです。
素人質問ですがletではなくvarを使うのはどのような意図があるのでしょうか？
４問目は"fetching"を定数化するべき。
ただ、比較しているのが「state」なので、
const STATE = {
// ・・・
FETCHING: "fetching",
// ・・・
};
と定義するとよし。
これだけだと
STATE.FETCHING = XXX;
で書き換えられるけど・・・
後、一見
if (isEmpty(listNode)) {
if (importData.state === "fetching") {
// ・・・
} else {
// ・・・
}
}
とした方がいいように見えるが、元のコードの最初のif文ブロックの中身が引数の中身を書き換えるような実装だった場合に挙動が変わってしまう可能性があるので注意が必要。
詰めてるわけじゃないって何度も言ってくれて優しいなー。こういう先輩がいる職場で働きたかった。
レビューなんかせずにそのまま結合入って障害出たら今日中に全部見直せ、とかそんな環境ばかりだった😫
createHogeのやつ、修正前と修正後で挙動が変わるのでは・・・？
修正前は
createHoge();
と
createHoge("");
は同じ挙動を示すけど、修正後はこれらが違う挙動になるかと。
しれっと呼び出し箇所も空文字から引数無しに変わってるけど、「要するにリファクタリング」と言う問題でそういった事をするのはよろしくない。
こうなると関数の仕様の話になってくるから、本題から逸脱してしまうかと。
「処理の意図をコメントで残す」と言う事も大切だな・・・と思った。
多分こういう修正ミス（「呼び出し側までわざわざ書き換えるのはどうなの？」と言うツッコミはさておき）って実務でも起こり得るとは思うけど、例えば
// 空文字は許容しないのでhogehogeに置換する
と言ったコメントがあれば防げるとは思う。
・・・つい昨日、実務で
hogeFunc(0);
function hogeFunc(hoge) {
if (!hoge) {
// ・・・・
}
}
と言うコードを見て一瞬引っかかったのを思い出したｗ
問3同じことを思いました。前と後でnullや空文字などを渡した時の挙動が変わってますよね。
0以下なら0にする冗長なので
return max(0, eval)
bigger than は値を含まないから、関数名を正確にするなら age > LEGAN_AGEであってage >= LEGAl_AGEではないと思います
最後のコードですが、negativePointに入れる値は正なのか負なのか、
NEGATIVE_RATEは掛けるのか割るのか、
後で使う人にとってわかりにくいなと思いました。
コメントの話ですかね。
この企画おもろい！
10:00 のコード太古の業務システムとかに実在してそうで怖い
GPLコードを使うのでソース公開しなきゃいけないけど、誰にも理解させたくない。
という大人の事情なのかな？と思いました。
4問目は、isEmpty(listNode)を2回判定してるけど、最初に判定しとけば1回で済む。
6問目は、端数誤差をどの程度許容するのか分からないけど、除算の結果を端数整理しないと小数点以下の値が意図しない桁数まで表示されたりしそうで気持ち悪い。
第３問 改善後に7行目の関数呼出の実引数を変えるのはあり？結果は変わらないかも知れませんが。
関数の挙動が変わってしまっている（＝リファクタリングの結果バグを埋め込んでいる）のでよろしくない。
消したのが文字列リテラルだからまだいいものの、どんな値を取っているか分からない変数だとしたら消せる訳がない。
なるほど！と思うことが多いですね！python版もやって欲しい！
大学でCを初めてやってるけど人とは違うコード書ける予感がしてきた
プログラミング、、おもしろい、、!?
これ、LIVEでやったら面白そう
一問目で言うと、constとかの以前にNAMEっていう変数名を考え直せっていうのが一番では？
為になります。より長めのクソコード版もあったら嬉しいです。
isOgawaのNAME定数は逆に分かりづらいので、リテラルと直接比較するか、関数内に入れ込みますね
クリーンアーキテクチャとか言われるとビクビクするけど、こういうクソコード企画は面白いです！クソというよりアホコードってイメージです！（笑）
いい勉強になります！
Liveでクソコード皆で直す企画とかやって欲しいです。
おがりょーこわすぎw
秋田から応援しております
クソコードを書かないためのテクニック……JavaScriptではなくTypeScriptを書く
１，仕様にもよりますが、グローバル定数は被りやすいので長い方がよろしいのではないでしょうか？NAME_PERSON_OGAWAみたいなのはいかがでしょうか？
５，var engineerEbaluation=frontendSkillPoint+backendSkillPoint-(negativePoint / NEGATIVE_RATE);
にすると一時変数が無くなって、エンジニアらしいスッキリしたコードになるのではないでしょうか？
すいません、Javaスクリプトは実は知らないのですが、知らない人が見ても判りやすい題材を持ってきた、入れ食いを狙った巧妙な動画ですね。まんまと食いついてしまいました。
５は 私もそう思いました
５に関しては多分現場次第にもなってくるかとは思われる。
ただ、「スッキリしたコード」と言うのであれば、そもそも変数に取らずに
return Math.max(frontendSkillPoint + backendSkillPoint - (negativePoint / NEGATIGVE_RATE), 0);
で済む。
いやー確かにそうですね。エンジニアのコードですね。
前田くんかわいいー😳
2つ目のやつ、引数をageじゃなくuserにした方が使いやすい気がしたんだが…。
定数は大文字で書く・・・覚えます！😂
すみません。JSとは全く関係ないのですが、HTMLで写真をinsertするときの写真のURLというのはどうやって作るのでしょうか？
みたいな感じです。
再代入はバグの温床なのでステート管理するレイヤ以外では基本的に NG でイミュータブルな実装を心がけるように伝えています
どうしても避けられない時はその理由をコメントに添えていなければコードレビューで弾いてます
js, ts の場合はもはや var は絶対 NG でどうしても必要な時は let を使います
共通処理があった時に内部関数に切り出すこと多いですが、共通処理なのであればそれはひとつの責務と言えることが多いので内部関数化するよりも先にクラスに切り出すことを考えるように伝えています
varを使ってる時点でクソコード
最後の問題三項演算子を使うとif文書かずに済みます
```
return engineerEvaluation < 0 ? 0 : engineerEvaluation
```
🇯🇵🇯🇵🇯🇵いいですね
biggerThanの場合はそもそも○○より大きいという表現なので>=ではなく>を使うべきでは
クソコード偏すきです。ｗｗ おもしろい×学び 最高です。Javaもぜひ～～～
物凄いクソコードでしたw
タイトルがjavascript編ではありますが、特異性についてあまり触れてないですね
初心者向けと書いてあるので見やすい動画にしているのかもしれないですが、
fetchやアロー等も含めると面白いかもしれないですｗ
自分が見てすぐ思ったのは｢name｣と｢NAME｣で変数名が同じで分かりにくいなーと思ったんだけど、
違うのかな
ogawaと判定するための定数だから｢OGAWA_NAME｣とか｢CHECK_NAME｣とか(変数名の作り方は苦手ですが)にした方がいいかなーと
あくまで初心者向け
ワイ
var calcEngineerEvaluation(frontendSkill, backendSkill, negative) => Math.max(0, frontendSkill + backendSkill - negative / NEGATIVE_RATE);
クソコード見て笑っちゃった自分はちょっとだけ勝ち組
実業務でクソコード、めちゃくちゃ書いてるなーー、、反省😂
某技術書に影響されたのでしょうが、「クソコード」という呼称はやめましょうよ・・・「良くないコード」とかでいいのに。