Loading deck

$nl

〜僕が見た"最高"のPHPコード〜

黒曜

@kokuyouwind

▲ スライドのリンクは常に表示されています

  スライドめくりもリンクされます

$ whoami

  • 黒曜 / @kokuyouwind
  • 株式会社Misocaに転職して半年
  • 現職: Ruby on Rails/React
  • 前職: PHP/Scala
  • 趣味: OCaml

宣伝

プログラマ向けの謎解きゲームを作りました

宣伝

本題

RubyKaigi2015

私が人生で見た

最悪のRubyコード

聞きに行った

感想

一つ思ったこと

自分が見た
PHPコードは
これより酷かった!

(主観)

$nl

〜僕が見た"最高"のPHPコード〜

黒曜

@kokuyouwind

構成

独自フレームワーク!

構成

構成

/core

    /model - MVCのM

    /logic - 共有ロジック  

/for_xxx

    /public - web公開ディレクトリ

    /controller - MVCのC

    /view - MVCのV

    /template - Smartyテンプレート

構成

/core

    /model - MVCのM

    /logic - 共有ロジック  

/for_xxx

    /public - web公開ディレクトリ

    /controller - MVCのC

    /view - MVCのV

    /template - Smartyテンプレート

controller

  • MVCのC…ではない!
  • 大体のメイン処理を書く場所
  • 変数名が大体ひどい
    • 後述する
  • 例外駆動プログラミングという技法がフル活用される
    • 後述する
  • とりあえずひどい
    • 後述する

構成

/core

    /model - MVCのM

    /logic - 共有ロジック  

/for_xxx

    /public - web公開ディレクトリ

    /controller - MVCのC メイン処理を書く場所

    /view - MVCのV

    /template - Smartyテンプレート

view

  • MVCのV…ではない!
    • そもそもテンプレートは別にある
    • controllerから受け取った値を表示用に加工する?
      • 大体は受け流すだけ
      • たまにネストのレベルを変えてて悶絶する
  • できることはControllerとほぼ一緒
    • たまにすごい処理が書いてあってビビる

構成

/core

    /model - MVCのM

    /logic - 共有ロジック  

/for_xxx

    /public - web公開ディレクトリ

    /controller - MVCのC メイン処理を書く場所

    /view - MVCのV 

    /template - Smartyテンプレート

model

  • MVCのM…ではない!
  • 配列を上手く扱うためのクラス
    • ドメインモデルのデータはすべて配列で扱う
    • (PHPの配列は連想配列)
  • DBとの入出力も扱う
    • PDOでクエリを直接投げる
    • 独自のクエリビルダーが4種類くらいある
  • どこからでも$this->getModel('Name')で取れる

model

<?php

class UserModel extends HogeFWModel
{
  public function get($id, $ignore_delete = false) {
    $table = $this->get_table_name($id); // ソフトシャーディング
    $query = "SELECT * FROM $table WHERE id = ?";
    if (!$ignore_delete) {
      // deletedが未来のことがあるので、IS NULLだけではだめ
      $query .= "AND (deleted IS NULL OR deleted <= '$(data('Y-m-d h:i:s'))'");
    }
    $records = $this->db->execute($query, $id);
    if ($records === false) {
      // 厳密にfalseならSQL接続エラー
      throw new SQLException('SQL接続エラーです');
    } elseif (!$records) {
      // クエリは成功したが中身が空
      throw new NotFoundException('ユーザーが見つかりませんでした');
    }
    $user = $records[0];
    // 他に必要な情報を補完する
    return $this->appendInfo($user);
  }
}

model

// こんな感じで使う
// callはメソッドの呼び出し結果をキャッシュする関数
$this->cache->call('User', 'get', $id);

// callには配列形式で渡しても良い
$um = $this-> getModel('User');
$this->cache->call([$um, 'get'], $id);

// 返ってくるのは配列(というかハッシュ)
['id' => '758',
 'name' => '黒曜',
 'registered' => '2016-04-16 15:50:00', // 大抵MySQL形式
 'last_login' => 1460789400, // たまにUnixTimeが入っている
 'attributes' => [
   'belongs' => [
     'Misoca' => ['id' => '331'] // どんどんネストする
   ],
   'is_penalty' => 'none', // なぜか真偽値ではなく文字列
 ],
 'append_info' => [
    // 3段くらいネストしたなんらかの情報
 ]
]

構成

/core

    /model - MVCのM 配列の加工とDB処理

    /logic - 共有ロジック  

/for_xxx

    /public - web公開ディレクトリ

    /controller - MVCのC メイン処理を書く場所

    /view - MVCのV 

    /template - Smartyテンプレート

logic

  • 共有ロジック…ではないものがほとんど
  • 大体はcontrollerの中身を分割したもの
    • バージョン移行の際に、controllerの処理を
      ​まるまるコピーしlogicクラスができたりした
  • controllerでできることはlogicでもmodelでもできる
    • たまにlogicやmodelから直接viewに渡す値を
      設定していてビビる
  • どこからでも$this->getLogic('Name')で取れる

構成

/core

    /model - MVCのM 配列の加工とDB処理

    /logic - 共有ロジック メイン処理を書く場所2

/for_xxx

    /public - web公開ディレクトリ

    /controller - MVCのC メイン処理を書く場所

    /view - MVCのV 

    /template - Smartyテンプレート

"最高"のPHPコード

"最高"のPHPコード

  • 単一のコンテンツを表示するページのController
  • メイン処理のメソッドが3000行くらい
    • 1メソッドで3000行
    • うち2000行くらいがcatch節
  • 5段くらいにネストした100項目くらいの配列を構築
    • これが1つのコンテンツを表す
    • とりあえずvar_dumpしてデバッグする
  • 5環境くらいにforkして、それぞれ独自に進化している

2文字変数

変数名が酷い

<?php

class ContentController extends HogeFWController
{
  public function execute() {
    // 大体2文字変数
    $cm = $this->getModel('Content');
    $nl = $this->getLogic('Normalize');
    $cont = $this->cache->call([$cm, 'get'], $this->getAttribute('id'));
    $cont = $nl->norm($cont);
    // 間に300行くらい挟まる
    if($has_body) {
      $cont = $nl->normBody($cont);
    }
    // 間に500行くらい
    if ($need_next) {
      // さっきの$nlを上書きする
      $nl = $this->getLogic('NextContent');
      $content['next'] = $nl->calc($content);
    }
    // ...まだ続きます

変数名が酷い

    // ...続き

    // 間に300行くらい
    // さっきの分岐を通ってると死ぬバグ
    $content['append'] = $nl->normAppend($content);
    //間に200行くらい
    if ($need_extra) {
      // 明らかに$nlじゃないけど$nlを上書きする
      $nl = $this->getLogic('Extra');
      $content['extra'] = $nl->compute($content);
    }
    // 間に300行くらい
    // viewにcontentを受け渡す(Model, Logicからも可能)
    $this->setAttribute('content', $content);
    return;
  }
}

例外駆動プログラミング

例外駆動プログラミング

<?php

class ContentController extends HogeFWController
{
  public function execute() {
    try {
      $cm = $this->getModel('Content');
      $content = $this->cache->call($cm, 'get', $this->getAttribute('id'));
      if(!this->user) {
        throw new NotLoginException();
      }
      // ログイン時、コンテンツありの処理が1000行くらい続く
    } catch (NotLoginException $e) {
      // 非ログイン時の処理
      if ($cm->isAllowGuest($content)) {
        // 非ログイン時、コンテンツ閲覧可の処理が800行くらい続く
        // うち600行くらいはログイン時処理のコピペ
      } else {
        // view/content_error_viewを描画する
        return 'content_error';
      }
    }
  }
}

どうしてこうなったか

  • そもそも2007年ごろから作られ始めたコード
    • PHP5.2の頃で名前空間すらない
    • モダンで実績のあるフレームワークもあまりない
  • 初期は数名での開発だった
    • レビュー文化のないカウボーイプログラミング
    • テスト文化もない
  • 新機能や企画優先で継ぎ足し実装が横行した
    • 品質より納期を優先するスケジュール
    • 追加された機能が消せず、膨れる一方

どうやって戦っていたか

  • レビュー文化が根付き始めた時期の入社だった
    • 当時はsvnでFisheye/Crucibleを使っていた
  • PHPUnitも先輩が広めていた
    • 複雑すぎてテスト不可能な部分が多すぎた
    • 新規に書くコードはテストを書く、という共通認識
    • 手作業確認によるリファクタリング(リスク高)
  • grep力を高めて物理で殴る
  • 最近はScalaで書きなおそうとしている(はず)
    • 転職したため現状は謎

教訓

  • 機能追加偏重の文化は保守不能なコードを生み出しうる
    • 無論、ユーザーに価値を届けることは大事
    • うまくバランスを取ることが重要
  • レビュー文化、テスト文化で品質を保つ
    • スピードを上げ過ぎない「足枷」としての機能
  • 無理な納期から開発チームを守る防火壁が必要
  • 機能もコードも「捨てられる」ように作る
    • 犠牲的アーキテクチャ
    • 機能テスト

予備スライド

他に面白かったコード

  • バグにバグが重なって上手く動いているコード
    • 1日が 60 * 60 * 60 * 24 秒ある
    • 1週間が 8 日ある
  • コメントと返り値が真逆
    • 関数名は block
    • 「過負荷ブロックされたらfalseが返ります」
    • 実際は過負荷ブロック時にtrueが返っていた
  • forが3つネストした中に「continue $n;」
  • 100行くらいあるメソッド先頭に「return false;」

構成

/core

    /model - MVCのM

    /logic - 共有ロジック  

/for_xxx

    /public - web公開ディレクトリ

    /controller - MVCのC

    /view - MVCのV

    /template - Smartyテンプレート

public

  • webからのエントリポイント
    • apacheでDirectoryとして指定される
  • Controllerをkickするphpファイルが配置される
    • URL routesの数だけファイルが置かれる
    • Router? なにそれおいしいの?
  • 画像/js/cssなどの静的ファイルもここに配置される

構成

/core

    /model - MVCのM

    /logic - 共有ロジック  

/for_xxx

    /public - web公開ディレクトリ

    /controller - MVCのC メイン処理を書く場所

    /view - MVCのV 

    /template - Smartyテンプレート

template

  • Smartyのテンプレートを置く場所
  • ネストが非常に深い
    • 3重ifの中身が数十行とかはざら
    • ロジック分離? なにそれ美味し(ry
  • 多言語対応の影響でさらにカオスになった
    • 日本語以外では出さない、みたいな分岐が
      そこら中にある