引数の数を考慮する。また、登場人物の数を考慮する。

引数の数を考慮する。また、登場人物の数を考慮する。引数が多いメソッドを見るとモヤモヤします個人的には引数に関しては基底クラスのメソッドは4つまで、それ以外は3つまでと定めていますｰ 4つの原点はPerl Best Practiceだったのではないかと記憶して…

2018-01-11

変数展開したSQLは常にインジェクションが発生すると思い込もう

変数展開したSQLは常にインジェクションが発生すると思い込もうこういうコードを見ると不安になります PHPでかつ、Laravelであると想定します $foo = SomeModel::select("select * from some_table where some_column = {$condition}"); 何がよくないのか？…

2018-01-07

3度目のコピペで共通化する

3度目のコピペで共通化する間が空いてしまいましたがこの記事ですただし、これって何でこう思ったんだっけ？という裏をとれなかった記事です正直、ちょっとモヤモヤしつつ書いています。。何がよくないのか？ 3度もコピペされるコードは共通化、もしくは…

2018-01-04

Wikiに書こう

Wikiに書こう今回はレビューとは関係ない話になりますふと、チケットを見ていると調査内容をExcelにまとめている方がいて少し驚いたので書いてみます何がよくないのか？ Excelは基本的に有料ソフトですまた、立ち上がりまでに多少の時間を要求しますダ…

2018-01-03

配列には同じ型のものだけを格納する

配列には同じ型のものだけを格納する調査中に下記の様なコードを見るとツライ気持ちになったりします array(2) { [0]=> array(1) { ["foo"]=> int(1) } [1]=> object(stdClass)#1 (1) { ["foo"]=> int(2) } } 何がよくないのか同様の値を保持している様に…

2018-01-02

計算量を常に意識しよう

計算量を常に意識しようこういうコードを見るとパフォーマンスが劣化しないか気になります foreach ($foo => $bar) { if (!in_array($bar, $some_list)) { continue; } // 何かの処理 } 何がよくないのか？ in_array は毎回、配列内を検索します今回はPHP…

2018-01-01

安易なgetter/setterを作らない

安易なgetter/setterを作らない明けましておめでとうございますこういうコードを見るとモヤモヤします if ($foo->getDate() > $today) { return false; } 何が良くないのか？ $foo->getDate() を使って条件判定を行っている箇所が正しくカプセル化を行えて…

2017-12-30

伝わらない命名をしない

伝わらない命名をしない名前はとても大事なものです、ロジックに反した名前を付けてしまうとレビュアーは？となります『プログラマが知るべき97のこと』の日本語訳版でmatzさんも寄稿されていましたねどうやるか？ Google翻訳を使わない本質的な名前を導…

2017-12-29

問題を解決するためだけにコードを書く

問題を解決するためだけにコードを書く当ブログで一番伝えたかったことですコードを書くのは最終手段コードは必然的に書かなければならない時にのみ書かれるべきですなぜならば、不要なコードは常に単なる複雑さを提供するだけのものだからですバグを生…

2017-12-29

ライブラリのコードを隠蔽する

ライブラリのコードを隠蔽する私は今、PHPでLaravelを使うプロジェクトに参加していますジョイン当初に驚いたのは下記の様なコードが散見されていたことです return response()->json($result); 論点は2つライブラリに致命的な脆弱性が見つかった際に徹夜…

2017-12-28

条件分岐の順番にも気を使おう

条件分岐の順番にも気を使おうこういった条件分岐を見るとモヤモヤします if ($foo->exists()) { // DBにレコードがなければFALSE return false; } elseif (isset($parameter['bar'])) { // パラメータにbarが含まれなかった場合はFALSEを返す return false…

2017-12-27

例外を拾おう

例外を拾おう正確には正しく拾い、投げ捨てることをしない様に努めましょうこういうコードを見るととてもモヤモヤします。そして不安になります try { // 例外が発生するかも知れない処理 } catch (Exception e) { } 何がよくないか？まず、例外が発生し…

2017-12-26

UNIXコマンドを味方にしよう

UNIXコマンドを味方にしよう今日はレビュー自体の話でなく、レビューを依頼する際にしておきたい事前準備の話をしたいと思います IDEやGUIツールを窓から投げ捨てよう IDEやGUIツールは非常に便利です。否定はしませんただ、こういったツールには一つの弱…

2017-12-25

コードフォーマッタを使おう

コードフォーマッタを使おうインデントや、スタイルの指摘は機械に任せようという話規約を破るのが人間コーディング規約に沿わないコードを書くこともあるコーディング規約にないからと時代遅れも甚だしいコードを書くこともあるうっかり、スペースを消…

2017-12-24

トランザクション内で行うべきことは更新処理のみ

トランザクション内で行うべきことは更新処理のみトランザクションを開始してから更新対象を取得している処理モヤりませんか？どうしてそう思うのか？トランザクションはアトミックな処理を保証するもの一方でアトミックでない処理にならない様にロック…

2017-12-23

目的と効果を明らかにする

目的と効果を明らかにするレビュー依頼は依頼だよねという話依頼時あるある口頭でお願いされる少なくともチャットなりのテキストに残しましょう障害対応のhotfixなら仕方ないけど、それでも後でテキストに残しましょうお願いしますだけ Aさん: これレ…

2017-12-23

長いPRは避ける

長いPRは避ける新規開発とかをしていると起こりがちなのが数千行にも及ぶ差分数千行ものコードを正しくレビューできる人間なんてそうそういない少なくとも自分は何かを見落とす自信があるどうすればよかったのか？機能ごとの開発に切り分けるそれでも…

2017-12-03

不等号の向きは合わせる

不等号の向きは合わせるこういう複合条件の場合に頭の中で反転させることをなくしたい if ($i > 1 && $i < 100) { // 何らかの処理 } この様に書くと直感的になる if (0 < $i && $i < 100) { // 何らかの処理 } これもPBP(Perl Best Pracitice)に書かれてい…

2017-11-17

否定のELSEは基本的に避ける

否定のELSEは基本的に避けるこういうのを見るとモヤモヤします if (!empty($foo)) { // 正常処理 } else { throw new \Exception('foo is empty'); } 理由 !emptyで空でない時という条件を指定しておきながら、逆説のELSEをとってしまうところたしか、これ…