こんなMR（PR）出しちゃダメなんだZ！！

転職して半年経ちました

未経験からエンジニア転職して早くも半年が経ちました。
界隈では未経験転職から1年半〜経って転職ドラフトとかで年収アップしてる方が散見されてきましたが私は今の所、全くそんな兆しは見えません。

今回はそんな自分の恥を晒します。
今までに出してきたMR（マージリクエスト）で
先輩エンジニアの方々にご指摘を頂き、先輩の貴重なリソースを奪ってきた数々の
クソコードのうち、選りすぐりの指摘をランキング形式でお届けします！

ちなみにマージリクエスト(MR)やプルリクエスト(PR)というのは自分のコードを
実際に動いている本番環境やその手前のステージング環境に混ぜてもらう為に
権限を持ったエンジニアに確認してもらう事です。

とても大事な行為ですね！
Github使ってるかGitlab使ってるかで呼び方変わりますがやってることは変わりません。

しょーもない事でパイセン達の時間を割かないように是非この記事を反面教師にして気をつけてください！

では早速見ていきましょう！

第5位 「ちょ、これ、なくても動くんじゃね？」

最初の方に作ったメソッドが実装していくうちに何故かどこからも呼ばれていなかったり、
変数になんか値を突っ込んでみたけど、その変数結局使わなかったり、デバッグ用にログ出力したりしてそれを消し忘れたりですね。
この様ないらない変数宣言やメソッドがあるとコードの可読性を下げ、自分を含む読み手をただただ混乱に陥れるだけのテロ行為であると認識しましょう。認識します。

第4位　「ちょ、ログの出し方適当過ぎじゃね？」

以下、実際に自分が出した無謀なMRの一部です。チーム開発を舐めてる。。。

sample.rb

Rails.logger.debug "入ってるよDELETEーーーーーーーーーーーーーーーーーーーーーー"

いや、いくらデバッグだからって適当過ぎでしょ。
せめて対象のパラメーターとかなんかヒントになるもの出そうぜ！

「コメントだけじゃわからないから対象データもちゃんと渡さないといけなんだ！」と学んだ私は以下のMRも出しました。

sample.rb

return puts "どれかの処理でエラーが起きました。配列を確認してください。=> #{@res_ary}"

すごい！
大量の配列をエラー文に丸投げ！
そしてその中のどれかでエラーが発生したかを探させるという傲慢さ！
「どれかの処理」って軽くクイズ出してんじゃねぇよ！エンジニア辞めちまえ！

以下の様な感じでログ出すと親切ですね。
①エラー内容
②期待される結果
③実際の処理結果
④その後の対応策
これらを配列で返してあげたり。

sample.rb

err_ary = {
      "エラー内容" => e,
      "期待される結果" => "S3へ#{file_name}を保存",
      "処理結果" => "#{file_name}をS3へ保存できずに処理を終了しました。",
      "対応" => "エラー内容を確認して修正後、もう一度バッチを実行するか直接S3から削除してください"
        }
    @config.logger.error "エラー発生!エラー詳細------->#{err_ary}"

第3位　「ちょ、変数名、メソッド名センスなくね？」

以下、私の秀逸なメソッド名

sample.php

public function update_agency_put_db(pamams)

updateかputどっちかにしたら？

sample.rb

def delete_or_update_from_dynamo(params)

削除したいの？updateしたいの？笑
何したいのかわからないメソッド名。さすがです。

ちなみにリーダブルコード読了済みです。
読んだ事に満足してしまう愚かな人間。

「そのメソッドは一言で表すと何をするのか？」
この答えが「DBからデータ取得してデータをCSVにしてS3に保存して〜〜〜」
みたいに一言で説明できないならクソメソッドです。
できるだけ単一責任にしましょう。
上記で言うなら
「DBからデータを取得するメソッド」
「CSVを生成するメソッド」
「S3に保存するメソッド」
に分けたほうが疎結合となり、バグも発見しやすいし変化にも柔軟に対応できます。

第2位　「ちょ、ハードコードやめて？」

ハードコードとはソースコードの中に「固定値」をそのまま書き込むことです。
流石にAWSの認証情報とかはそのまま書き込むことはないですが、定数として設定されてるものや
configで管理されてるものはハードコードせずにちゃんとそちらから使いましょう。
以下に簡単な例を記します。

gender.rb

class Gender
 MAN = 1
 WOMAN = 2
end

sample.rb

 # NG
 User.where(gender_id: 2)
 # OK
 User.where(gender_id: Gender::WOMAN)

新しくプロジェクト入ってきた人からするとgender_idが2って何のことかわかりませんね。
定数使いましょう。
また、既存PJでも自分が分かりずらいと思う部分は変更しても良いと思います。

第1位　「ちょ、インデントちゃんとして？」

これはもう、拡張機能とか入れて未然に防げって感じですね。
でも最初は結構やらかしてしまって堂々の一位とさせて頂きました。

これらのご指摘は個人開発しかやってこなくて自分の書いたコードが動く事に喜びを感じて
「とりあえず動けば良し！」という自己満に浸っていた為当然ですね。
同じことを何回も指摘されないように気をつけます。

あとはタイポも結構やりがちなのでVSCodeなら「Code Spell Checker」とか拡張機能使って防ぎましょう！

これからエンジニアになる方の参考に少しでもなればと思います。

少し前の時代ならぶん殴られてるんじゃないかな？と思いますが、皆さん優しく指摘してくださるので良い職場です。