【初心者向け】クソコードを書かないためのテクニック【JavaScript編】

Поделиться
HTML-код
  • Опубликовано: 26 ноя 2024

Комментарии •

  • @yoshito323
    @yoshito323 2 года назад +12

    最後のやつは「JavaScript標準の関数も使えばシンプルに書けるよね」と言う事も言える。
    8行目 ~ 12行目は
    return Math.max(engineerEvaluation, 0);
    でOK。

  • @mng6501
    @mng6501 2 года назад +31

    userの構造体はageじゃなくて生年月日が汎用性高い

  • @yoshiki6375
    @yoshiki6375 2 года назад +7

    第一問:isOgawa という関数であれば、判定基準が "ogawa" 以外無いので、そもそも定数にする必要ないと思う。加えて、"ogawa"以外が引数の時ダンマリなのは少し悲しい。
    第二問:biggerThan〜だと'より大きい'になるので'>='より'>'を想像してしまうから関数名を変えるべき。
    第三問:...(デフォルト引数か…)
    第四問:むしろネストを一つ深くする方がわかりやすい。
    まず、if(isEmpty(...)){
    この中でisFetchingという関数をつくりifで再分岐する。つまり、
    if(isEmpty(listNode)){
    if(isFetching(importData.state)){
    //...
    }else{
    //...
    }
    という感じ。

    • @yoshito323
      @yoshito323 2 года назад +5

      4問目に関しては、その修正をした場合に若干挙動が変わってしまう点は注意が必要。
      処理そのものが省略されているので何ともだが、前の判定と後の判定がそれぞれ別で通るように実装されている事に何かしらの理由があった場合にその修正はマズい。
      (そういう作りがそもそもクソと言うツッコミはさておき・・・w)

    • @yoshiki6375
      @yoshiki6375 2 года назад +2

      @@yoshito323
      まあ、確かにそうですねww
      1つ目の if 文のなかで importData.state とかが変わる可能性がありますね(!?)前田さんが else if の話とかしてたのでそれに引きずられた感じですね。
      とはいえ、無意識的にこう書き直してしまったということは、そもそも問題のあるコードであったということですかね(>人<;)

  • @MUSCLEMUSCLEPIG
    @MUSCLEMUSCLEPIG 2 года назад +3

    こういった企画大好きです。動画長かったけど、全部見ました。

  • @ああ-l5z1g
    @ああ-l5z1g 2 года назад +8

    最後のコード
    再代入するという明確な意義があるのであればvarよりletを使った方がいい気がしますが...
    そのあとで参照を目的とした新たな変数をconstで用意してあげるのがベストな気がします。

  • @1u169
    @1u169 Год назад +1

    エンタメ & 学び って最高だと思います!

  • @chihayafurune
    @chihayafurune 2 года назад +12

    2問目、bigger than だと「より大きい」なので等号入ってるのも違和感のような。

    • @valmanway3800
      @valmanway3800 4 месяца назад

      同じことは思いました。
      Bigger thanという表記だと、「より大きい」という意味の方が正確なので、
      return age > LEGAL_AGE ;
      という意味としてとらえられてしまうのではないのかなと思いました。
      ついでにどうでもいいことをいうと、年齢を対象にするのであれば、Biggerよりもolderのほうがわかりやすいかなっと。(でも結局isLegalAgeが最適だよねとはなりますがw)

  • @yoshito323
    @yoshito323 2 года назад +9

    最初の問題、isOgawaと言う関数名は「真偽値を返す関数」と認識される可能性があるので変えた方がいい。
    でもいい命名が思いつかないw
    後、NAMEと言う定数名も、関数名と関連が無いのであまりよろしくない。
    (2問目の「LEGAL_AGEに変更が入ったら・・・」と同じ問題が起こる)

  • @otk3767
    @otk3767 2 года назад +5

    最後の問題、varよりconstにした方が良くないですかね?

  • @Gent-Owl
    @Gent-Owl 2 года назад +5

    ここじゃないどっかで「varは使わないで。letにして」みたいなこと言われましたね。
    scopeは可能な限り狭く、って。
    以降jsでは絶対にvarを使わないおじさんが誕生しました。

  • @ヒローシ
    @ヒローシ 2 года назад +3

    4問目は、listNodeが配列っぽい名前をしているので、
    「Nullかどうか?」と「空配列かどうか?」で処理を分けた方が良いと思いました

  • @gokeTV
    @gokeTV 2 года назад +6

    これは面白いですね!勉強にもなりますし、普通に考えて見て楽しかったです!
    そしてこういう問題思いつくのすごいなと思いました!
    僕もプログラミング系の話を動画にしていますが、
    例に挙げられてたあまり良くないネーミングはやっちゃいがちなので気をつけようと思いました。

  • @わらび-s9k
    @わらび-s9k 2 года назад +11

    個人的には変数名もLEGAL_AGEで関数名もisLegalAgeにしちゃうとイコールなのかなって思うこともあると感じました。
    比較の要素を関数名に入れたいなと思いました。(more thanとかupperとかoverとか?)

    • @hackenbacker
      @hackenbacker 2 года назад +10

      賛成です。isLegalAgeだとぴったり18歳じゃないとfalseな印象なので、この年齢は成人か?と言う意味で isAdult がいいんじゃないかと思いました。

  • @refrain_net
    @refrain_net 2 года назад

    個人的にはこーかな…
    ①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});
    ```

  • @t_abasco
    @t_abasco 2 года назад +5

    NEGATIVE_RATEは0.1にして
    (negativePoint * NEGATIVE_RATE)にしたほうが良さそう。
    悪い点は10%分だけ評価から引きましょうね~って意図だと思うので、率はかけたほうがわかりやすいのと
    仮に15%分にしましょうとなった際に割り算だと実現できない。

    • @yoshito323
      @yoshito323 2 года назад +2

      ついでに言えば、除算よりも乗算の方が処理的には高速。
      JavaScriptの場合はユースケース的にそこまで気にする事もないかも知れないけど・・・

    • @早稲田生暇人
      @早稲田生暇人 2 года назад

      @@yoshito323 なるほど!

    • @block1948
      @block1948 2 года назад

      減点評価はやめて加点だけにしましょう。ができませんね。

  • @tk-bv2sx
    @tk-bv2sx 2 года назад +2

    問4
    個人的には条件式自体も分けたいかなと思いました。
    状態増えた時に、毎回isEmpty書く必要ないと思いますので。
    その上で状態を判定するメソッドに分離した方がスッキリしそうです。

  • @さとうゆうや-f4v
    @さとうゆうや-f4v 2 года назад +1

    素人質問ですがletではなくvarを使うのはどのような意図があるのでしょうか?

  • @yoshito323
    @yoshito323 2 года назад +1

    4問目は"fetching"を定数化するべき。
    ただ、比較しているのが「state」なので、
    const STATE = {
    // ・・・
    FETCHING: "fetching",
    // ・・・
    };
    と定義するとよし。
    これだけだと
    STATE.FETCHING = XXX;
    で書き換えられるけど・・・

    • @yoshito323
      @yoshito323 2 года назад

      後、一見
      if (isEmpty(listNode)) {
      if (importData.state === "fetching") {
      // ・・・
      } else {
      // ・・・
      }
      }
      とした方がいいように見えるが、元のコードの最初のif文ブロックの中身が引数の中身を書き換えるような実装だった場合に挙動が変わってしまう可能性があるので注意が必要。

  • @ts-he7qw
    @ts-he7qw 2 года назад +6

    詰めてるわけじゃないって何度も言ってくれて優しいなー。こういう先輩がいる職場で働きたかった。
    レビューなんかせずにそのまま結合入って障害出たら今日中に全部見直せ、とかそんな環境ばかりだった😫

  • @yoshito323
    @yoshito323 2 года назад +4

    createHogeのやつ、修正前と修正後で挙動が変わるのでは・・・?
    修正前は
    createHoge();

    createHoge("");
    は同じ挙動を示すけど、修正後はこれらが違う挙動になるかと。
    しれっと呼び出し箇所も空文字から引数無しに変わってるけど、「要するにリファクタリング」と言う問題でそういった事をするのはよろしくない。
    こうなると関数の仕様の話になってくるから、本題から逸脱してしまうかと。

    • @yoshito323
      @yoshito323 2 года назад

      「処理の意図をコメントで残す」と言う事も大切だな・・・と思った。
      多分こういう修正ミス(「呼び出し側までわざわざ書き換えるのはどうなの?」と言うツッコミはさておき)って実務でも起こり得るとは思うけど、例えば
      // 空文字は許容しないのでhogehogeに置換する
      と言ったコメントがあれば防げるとは思う。
      ・・・つい昨日、実務で
      hogeFunc(0);
      function hogeFunc(hoge) {
      if (!hoge) {
      // ・・・・
      }
      }
      と言うコードを見て一瞬引っかかったのを思い出したw

    • @runceel0130
      @runceel0130 2 года назад +1

      問3同じことを思いました。前と後でnullや空文字などを渡した時の挙動が変わってますよね。

  • @mng6501
    @mng6501 2 года назад +3

    0以下なら0にする冗長なので
    return max(0, eval)

  • @dangomushi983
    @dangomushi983 Год назад

    bigger than は値を含まないから、関数名を正確にするなら age > LEGAN_AGEであってage >= LEGAl_AGEではないと思います

  • @block1948
    @block1948 2 года назад

    最後のコードですが、negativePointに入れる値は正なのか負なのか、
    NEGATIVE_RATEは掛けるのか割るのか、
    後で使う人にとってわかりにくいなと思いました。
    コメントの話ですかね。

  • @fr11828
    @fr11828 2 года назад +1

    この企画おもろい!

  • @shm_7EC
    @shm_7EC 2 года назад

    10:00 のコード太古の業務システムとかに実在してそうで怖い

    • @block1948
      @block1948 2 года назад

      GPLコードを使うのでソース公開しなきゃいけないけど、誰にも理解させたくない。
      という大人の事情なのかな?と思いました。

  • @ぷにょーん
    @ぷにょーん 2 года назад +1

    4問目は、isEmpty(listNode)を2回判定してるけど、最初に判定しとけば1回で済む。
    6問目は、端数誤差をどの程度許容するのか分からないけど、除算の結果を端数整理しないと小数点以下の値が意図しない桁数まで表示されたりしそうで気持ち悪い。

  • @Nijist5_BR
    @Nijist5_BR 2 года назад

    第3問 改善後に7行目の関数呼出の実引数を変えるのはあり?結果は変わらないかも知れませんが。

    • @yoshito323
      @yoshito323 2 года назад +1

      関数の挙動が変わってしまっている(=リファクタリングの結果バグを埋め込んでいる)のでよろしくない。
      消したのが文字列リテラルだからまだいいものの、どんな値を取っているか分からない変数だとしたら消せる訳がない。

  • @ryoheisteveaoki2463
    @ryoheisteveaoki2463 2 года назад +4

    なるほど!と思うことが多いですね!python版もやって欲しい!

  • @zolt55
    @zolt55 2 года назад +1

    大学でCを初めてやってるけど人とは違うコード書ける予感がしてきた
    プログラミング、、おもしろい、、!?

  • @tk-bv2sx
    @tk-bv2sx 2 года назад

    これ、LIVEでやったら面白そう

  • @dangomushi983
    @dangomushi983 Год назад

    一問目で言うと、constとかの以前にNAMEっていう変数名を考え直せっていうのが一番では?

  • @ganma5k
    @ganma5k 2 года назад

    為になります。より長めのクソコード版もあったら嬉しいです。

  • @sattoman
    @sattoman 4 месяца назад

    isOgawaのNAME定数は逆に分かりづらいので、リテラルと直接比較するか、関数内に入れ込みますね

  • @早稲田生暇人
    @早稲田生暇人 2 года назад

    クリーンアーキテクチャとか言われるとビクビクするけど、こういうクソコード企画は面白いです!クソというよりアホコードってイメージです!(笑)

  • @marukamison773
    @marukamison773 2 года назад +1

    いい勉強になります!
    Liveでクソコード皆で直す企画とかやって欲しいです。

  • @hamyamorange6994
    @hamyamorange6994 Год назад

    おがりょーこわすぎw
    秋田から応援しております

  • @Cat-pv1zz
    @Cat-pv1zz 2 месяца назад

    クソコードを書かないためのテクニック……JavaScriptではなくTypeScriptを書く

  • @otanoshimi4
    @otanoshimi4 2 года назад +2

    1,仕様にもよりますが、グローバル定数は被りやすいので長い方がよろしいのではないでしょうか?NAME_PERSON_OGAWAみたいなのはいかがでしょうか?
    5,var engineerEbaluation=frontendSkillPoint+backendSkillPoint-(negativePoint / NEGATIVE_RATE);
    にすると一時変数が無くなって、エンジニアらしいスッキリしたコードになるのではないでしょうか?
    すいません、Javaスクリプトは実は知らないのですが、知らない人が見ても判りやすい題材を持ってきた、入れ食いを狙った巧妙な動画ですね。まんまと食いついてしまいました。

    • @Nijist5_BR
      @Nijist5_BR 2 года назад

      5は 私もそう思いました

    • @yoshito323
      @yoshito323 2 года назад +3

      5に関しては多分現場次第にもなってくるかとは思われる。
      ただ、「スッキリしたコード」と言うのであれば、そもそも変数に取らずに
      return Math.max(frontendSkillPoint + backendSkillPoint - (negativePoint / NEGATIGVE_RATE), 0);
      で済む。

    • @otanoshimi4
      @otanoshimi4 2 года назад +1

      いやー確かにそうですね。エンジニアのコードですね。

  • @kenta1234567893
    @kenta1234567893 2 года назад +2

    前田くんかわいいー😳

  • @のぶ-x2k
    @のぶ-x2k 10 месяцев назад

    2つ目のやつ、引数をageじゃなくuserにした方が使いやすい気がしたんだが…。

  • @KentaroxKondo
    @KentaroxKondo Год назад

    定数は大文字で書く・・・覚えます!😂

  • @鈴木進次郎-k7d
    @鈴木進次郎-k7d 2 года назад

    すみません。JSとは全く関係ないのですが、HTMLで写真をinsertするときの写真のURLというのはどうやって作るのでしょうか?
    みたいな感じです。

  • @kuramapommel
    @kuramapommel 2 года назад

    再代入はバグの温床なのでステート管理するレイヤ以外では基本的に NG でイミュータブルな実装を心がけるように伝えています
    どうしても避けられない時はその理由をコメントに添えていなければコードレビューで弾いてます
    js, ts の場合はもはや var は絶対 NG でどうしても必要な時は let を使います
    共通処理があった時に内部関数に切り出すこと多いですが、共通処理なのであればそれはひとつの責務と言えることが多いので内部関数化するよりも先にクラスに切り出すことを考えるように伝えています

  • @みま-d1q
    @みま-d1q 2 года назад +4

    varを使ってる時点でクソコード

  • @村-p3i
    @村-p3i 2 года назад

    最後の問題三項演算子を使うとif文書かずに済みます
    ```
    return engineerEvaluation < 0 ? 0 : engineerEvaluation
    ```

  • @kakkoiinippon
    @kakkoiinippon Год назад

    🇯🇵🇯🇵🇯🇵いいですね

  • @theworldofsnails9126
    @theworldofsnails9126 Год назад

    biggerThanの場合はそもそも○○より大きいという表現なので>=ではなく>を使うべきでは

  • @ST-lm9zz
    @ST-lm9zz 2 года назад

    クソコード偏すきです。ww おもしろい×学び 最高です。Javaもぜひ~~~

  • @kou-u2o
    @kou-u2o 2 года назад

    物凄いクソコードでしたw

  • @MrREred
    @MrREred Год назад

    タイトルがjavascript編ではありますが、特異性についてあまり触れてないですね
    初心者向けと書いてあるので見やすい動画にしているのかもしれないですが、
    fetchやアロー等も含めると面白いかもしれないですw

  • @akhiro7944
    @akhiro7944 2 года назад

    自分が見てすぐ思ったのは「name」と「NAME」で変数名が同じで分かりにくいなーと思ったんだけど、
    違うのかな
    ogawaと判定するための定数だから「OGAWA_NAME」とか「CHECK_NAME」とか(変数名の作り方は苦手ですが)にした方がいいかなーと

  • @dangomushi983
    @dangomushi983 Год назад

    あくまで初心者向け

  • @kussytessy
    @kussytessy Год назад

    ワイ
    var calcEngineerEvaluation(frontendSkill, backendSkill, negative) => Math.max(0, frontendSkill + backendSkill - negative / NEGATIVE_RATE);

  • @早稲田生暇人
    @早稲田生暇人 2 года назад

    クソコード見て笑っちゃった自分はちょっとだけ勝ち組

  • @嵐を呼ぶバヌシオー
    @嵐を呼ぶバヌシオー 2 года назад

    実業務でクソコード、めちゃくちゃ書いてるなーー、、反省😂

  • @サモハンキンポ-q6f
    @サモハンキンポ-q6f 2 года назад +1

    某技術書に影響されたのでしょうが、「クソコード」という呼称はやめましょうよ・・・「良くないコード」とかでいいのに。