自己紹介

エイチームグループ子会社のエイチームライフデザインで、マネージャーをしています。福田龍(@ryu_f_web)です。

この記事について

issue確認やコードレビュー(GitHub: Pull Request, GitLab: Merge Request)を見る際の観点をまとめたもの
- 業務で直近3ヶ月間、自部署全てのコードレビューを最終確認させてもらっていた上で、よくあったケースのご紹介です
構成
- 【issue段階】陥りがちな思考の癖2点
- 【コードレビュー段階】重要度別　観点

【issue段階】陥りがちな思考の癖2点

Howの固定化、Whyの欠如
実装によるデメリット考慮漏れ

開発に入る前、つまりissue切られて仕様を確定する"要件定義〜設計"の段階で気付けるようにしたい
- 開発後だと機能全部作り直しになることも多く、損失が大きい
このあたりの記事が分かりやすい
- 要件定義の目的は「プロジェクト5W2Hを明確にして失敗を減らすこと」

1. Howの固定化、Whyの欠如

概要
- 機能Aを新規実装したい
- 機能Bに、Xという仕様変更をしたい
- に対し、それらを実装するありきで話を進めてしまわないようにする
事例
- 目的は書いてあるが、ビッグワードで抽象的すぎる
  - 目的：インシデント防止のため
    - 実装する機能へのブレイクダウンで論理が飛躍しており、なぜそのHow（機能実装）するという判断に至ったか不明瞭
      - 代替手段を検討する余地が生まれづらく、非効率や本質からズレた機能改善に繋がることも
- 根本解決になっていない
  - 入力ミス多発のため、○○が含まれていたら赤文字アラートを出す
    - そもそも入力ミス起きないようなバリデーション実装をすれば解決できるのでは？
      - 「赤文字」「アラート」「強調表示」あたりのHowが出てきた時は、警戒ワード
        
        基本的に思った以上に人間は文字を読まないし、アラートに慣れて意味を失う
    - これも結局のところ、なぜそういう事象が起きうるのかWhyの落とし込みが不十分
- 目的が形骸化している、ルーチンワーク依頼
  - いつも定期的にやっているこの依頼、お願いします
    - そもそもWebエンジニアを介さない手段が取れないか？
      - 外部ツールへの移行、CMS化等
- 「できる/できない」と、「やる/やらない」が区別できていない
  - これってできますか？
    - Webエンジニア視点だと、単に技術的に実現可能か否かを聞かれているように思える
    - 非Webエンジニア視点だと、「OKだったらこのまま機能改修の話進められる！」ぐらいの温度感のことがある
    - そのため、単にYes/Noだけ答えるのは控える
      - なぜそれを聞いてきたのか背景
      - Howが適切か（より良い別の手段の洗い出し）
      - 事業としてコストやリスクも考え、実現できるか否か
      - これらを必要に応じて伝えた方がいい場合も多い

2. 実装によるデメリット考慮漏れ

概要
- 機能実装でメリットばかり目について、デメリットが考慮できていない
  - 中長期で見た際、重大事故や技術的負債に繋がる可能性
事例
- システムの複雑度が大きくなりすぎる
  - テストコードを書くにしても、あまりにも複雑なロジックになる
  - 針に意図を通すような繊細なシステムを構築したところで、それを保守し続けることに多大なコストと、不具合リスクを抱え続ける
- セキュリティ上のリスク増大に対する対策考慮不足
  - パスワードをランダム発行ではなく任意発行に変更
    - ランダム発行のパスワードを割り当てていたが、使い慣れないパスワード強制によるデメリットが多いため、会員登録時に自身で設定できるよう変更
    - しかし、今まで起きなかった問題も発生するようになる
      - 安易なパスワードでも設定できる
        
        辞書攻撃に弱い
        
        パスワードのバリデーション見直しまで、セットで対応する必要がある

【コードレビュー段階】重要度別　観点

★3 必ず得てほしい観点
★2 少なくともチームで1人以上は持っていてほしい観点
★1 あると良い観点

★3 必ず得てほしい観点

理由：個人情報漏洩や法令違反といった、会社や事業、個人にとって甚大な影響を及ぼす事態を防ぐ
法令
- 厳密に白黒判定付けるのは難しいが、怪しいものに「法務へ確認済み？」と投げかけられるアンテナは必要
- 一例（網羅しているわけではない）
  - 個人情報保護法
    - 第三者提供
      - グループ会社含め、別の会社に渡す際の制限や保持期間等
    - 目的外利用
      - 個人情報取得時に同意を得た以外の目的で利用できない
    - 要配慮個人情報
      - 信条、病歴等の、特に配慮が必要な個人情報
  - 景品表示法
    - 根拠が示されていない、満足度No.1等
    - 優良誤認や有利誤認にならないように
      - 自社サービスや価格で数字を出す時は慎重に
  - 特定電子メール法
    - 宣伝メールは事前許可が必要（オプトイン）
    - 解除できる方法を明示した上で、機能提供が必要（オプトアウト）
  - 法人税法等
    - 撤退したサービスだから、使わなくなったサービスだからと、古いテーブルやレコード削除時に注意が必要
    - だいたい7年ぐらいは帳簿上残しておく必要がある
  - 消費税法
    - 総額表示が義務
      - 税抜き価格単体での表示はNG
セキュリティ
- SQLインジェクション
  - SQL発行ロジックの脆弱性を付いて、不正なSQLが発行される
    - リクエストパラメータを文字列結合したりせず、preparedStatementを使うこと
- CSRF
  - フォームの脆弱性を付いて、外部サイトから不正なデータ操作をする
    - CSRFトークンは原則OFFにしない
- 総当たり攻撃（ブルートフォースアタック）
  - 00000, 00001, 00002等と、少しずつ違うパスワードでログイン試行し、正解のパスワードを割り出す攻撃
    - 連続ログイン失敗ロック
- 辞書攻撃（ディクショナリーアタック）
  - 安易パスワードを許容してしまうと、00000やpassword等のよく使われるパスワードで不正ログインに繋がる
    - 安易なパスワード設定禁止
- 適切な暗号化、ハッシュ化
  - 個人情報等は暗号化（可逆）
    - 平文だと
      - 漏洩時の影響が大きい
      - BIツール等を導入している場合、社内スタッフが簡単に持ち出せる
  - ハッシュ化（不可逆）
    - パスワード等
      - パスワードは（システム管理者含め）本人以外知り得ない情報であるべき
        
        なので暗号化ではなく、不可逆なハッシュ化

★2 少なくともチームで1人以上は持っていてほしい観点

理由：保守性の低下等、技術的負債が増加するのを防ぐ
事例
- DRY原則違反
  - コピペのソースコード
  - 同一の定義が複数箇所で定義されている
- YAGNI
  - もしかしたら使うかも、といろいろ余計な実装になっていないか
- 「とりあえず」「一旦」の濫用
  - 警戒ワード。9割がた、その判断はやめた方が良い
    - そもそもスケジューリングや実装に無理がある
  - やるにしても、必ず明確に期限を切ってissue化する
- 循環的複雑度を低く保つ
  - やたら大きなメソッドには注意
  - CIで弾けると良い
- 困難は分割せよ
  - 原文から意味ズレるけれど
  - 1個のコードレビュー(プルリク/マジリク)、1個のリリースで無理にやろうとせず、最小限のスコープで徐々にリリースしていく