GameWith Developer Blog

GameWith のエンジニア、デザイナーが技術について日々発信していきます。

読みやすいコードを書くときに心がけていること #GameWith #TechWith

サービス開発部のdanaです。
先日アプリゲーム紹介詳細ページのリニューアルを行ったのですが、お気づきになられましたでしょうか?
リニューアルを行うにあたり、ゲーム紹介用のテキストから内容を抜粋して見出しテキストとして抽出/登録するコンバート作業を行いました。
その際の実装内容をベースに、コードを書く時に心がけている事をご紹介します!

記事のコンバート

前提としてGameWithの記事は詳しく公表していませんが、独自記法が存在しています。
独自記法を用いてライターさんがスピーディーに記事を作成し皆様に情報をお届けできるわけですね。
今回はこの記法が含まれた記事データをコンバートした際に工夫した箇所のお話になります。

コンバートの内容

読み込んだ記事データで下記の処理を実施します。

  • HTMLコメントアウトが含まれていたら削除
  • 特定記法1が含まれていたらその直前まででトリミング
  • 特定記法2が含まれていたらその直前まででトリミング
  • 特定記法3が含まれていたらその直前まででトリミング
  • 2枚目の画像が存在していたらその直前まででトリミング
  • 次の見出しまででトリミング
  • トリミングによってHTMLタグが閉じられていない場合追加
  • 見出しタグを置換

他にもいくつか存在するのですが、細かい説明を行う必要は無いので数多くの変換処理が必要という事だけ伝わればと思います。

コンバート仕様

HTMLコメントアウトは最初に削除するなどいくつかは優先度などがありますが、トリミング処理は順不同でOK。
最終的に最小のテキストになることが求められます。
また、確認のためどの内容でトリミングして最小の内容となったかログ出力を行います。

例として下記のような内容であれば、全てのコンバートを行うと見出しAから記法1直前まででトリミングされることが求められます。

見出しA
内容〜〜〜
内容〜〜〜
内容〜〜〜
記法1

記法2
画像

記法3
見出しB

実装方法

既存システムを用いるためphpにて開発。
コンバートは基本的にほとんど正規表現での置換となり、各コンバート毎に関数を作成しました。

愚直に実装した例

<?php

foreach ($articles as $article) {
    $converted_body = $this->_convert_body($article->get_body());
    $this->save($article, $converted_body);
}

private function _convert_body(string $body): string
{
    $latest_convert_message = "変換しませんでした";
    $body = $this->_delete_comment_out($body);
    list($body, $is_change) = $this->_trim_max_range($body);
    if ($is_change) {
        $latest_convert_message = "次の見出しでトリミングしました";
    }
    list($body, $is_change) = $this->_trim_tag_1($body);
    if ($is_change) {
        $latest_convert_message = "独自タグ3でトリミングしました";
    }
    list($body, $is_change) = $this->_trim_tag_2($body);
    if ($is_change) {
        $latest_convert_message = "独自タグ3でトリミングしました";
    }
    list($body, $is_change) = $this->_trim_tag_3($body);
    if ($is_change) {
        $latest_convert_message = "独自タグ3でトリミングしました";
    }
    $body = $this->_fixed_tag($body);
    Log::info($latest_convert_message);
    return $body;
}
private function _delete_comment_out(string $text) : string {
    return preg_replace('/<!--.*?-->/us', '', $body);
}
private function _trim_max_range(string $text) : array {
    $body = /* preg_replace(トリミング処理, '', $text);*/ ;
    $is_change = $text === $body;
    return [$body, $is_change];
}
private function _trim_tag_1(string $text) : array {}
private function _trim_tag_2(string $text) : array {}
private function _trim_tag_3(string $text) : array {}
private function _fixed_tag(string $text) : string {}

コードは非常に短いですが、見通しが悪いことが分かると思います。
特に$bodyを何度も再代入しているところや、毎回変更があったかどうかを確認してテキストを再代入している箇所が冗長ですし管理がしにくそうです。
仮に関数内で変更があったかを確認しようとした場合は、変更前と後を保持し続けなくてはならず、より混沌としたコードになってしまいそうです。

実際に作成したコード

実際に作成したコードは下記のような形です。(一部抜粋)

<?php

$convert_functions = [
    _delete_comment_out_func(),
    _trim_max_range_func(),
    _trim_tag_1_func(),
    _trim_tag_2_func(),
    _trim_tag_3_func(),
    _fixed_tag_func()
];
foreach ($articles as $article) {
    list($new_body, $complete_message) = array_reduce(
        $convert_functions,
        function ($params, $f) {
            return $f($params);
        },
        [
            $article_entity->get_body(),
            "変換しませんでした" // NOTE: デフォルトメッセージ
        ]
    );
    Log::info($complete_message);
    $this->save($article, $new_body);
}

private static function _create_resolver(\Closure $f, string $complate_message = "") : \Closure
{
    return function (array $params) use ($f, $complate_message) {
        $result = $f($params);
        return !empty($complate_message)
            ? static::_create_result($params, $result, $complate_message)
            : static::_create_result_update_body_only($params, $result);
    };
}

private static function _create_result(array $params, string $new_body, string $result_message) : array
{
    list($body, $complete_message) = $params;
    return [
        $new_body,
        ($body === $new_body) ? $complete_message : $result_message
    ];
}

private static function _create_result_update_body_only(array $params, string $new_body) : array
{
    list(, $complete_message) = $params;
    return [
        $new_body,
        $complete_message
    ];
}

private function _delete_comment_out_func(): \Closure {
    $f = function (array $params) {
        list($body, ) = $params;
        return preg_replace('/<!--.*?-->/us', '', $body);
    };
    return static::_create_resolver($f);
}

private function _trim_max_range_func() : \Closure {}
private function _trim_tag_1_func() : \Closure {}
private function _trim_tag_2_func() : \Closure {}
private function _trim_tag_3_func() : \Closure {}
private function _fixed_tag_func() : \Closure {}

実装するときに心がけたこと

ポイント1: コンバートする内容をパッと見で分かりやすくした

パッと見で全体的に冗長に見えます。
しかし配列の関数をarray_reduceで呼び出すことによって、呼び出される関数の一覧が即座にわかるようになっています。
愚直に実装した例だと、正しい関数を呼び出しているかを1行1行チェックしつつ、ifの中身と適合しているかも見る必要がありましたが、そういった点を意識する必要が無くなっています。

ポイント2: 複数値を返しつつ、コンバート処理に影響を与えないようにした

メッセージの書き換え部分を共通処理でラップしました。
ラップしたことにより変換関数自体は文字列を返却するだけの関数で良く、メッセージの書き換えの有無を気にする必要がなくなりました。

また、コンバート処理はクロージャで作成していますが、ドメイン層で作成して呼び出すように実装していたのであればユニットテストも容易になります。
全ての関数のインターフェイスが統一されればレビュアーへの負荷が下がるのではないでしょうか。
今回はcreate_resolverでパラメータそのまま渡していますが、実際に利用するテキストだけ渡すようにすることもできますね。

今回は使い切りのコードでしたので汎用性をそこまで高く実装していませんが、 関数を作成する関数までもが共通化出来れば、呼び出したい関数及びトリミング時のメッセージを渡した関数を$convert_functionsに追加するだけで利用可能になりますね。

ポイント3: 再代入を行わず直列に処理することを明確にした

array_reduceを利用したことで再代入が無くなりました。
今回の例だと処理が固まっているのでそこまで問題にはならなそうですが、関係ない場所で書き換えが発生してして予想外の問題が発生した、ということは皆さん1度は経験したことがあるのではないでしょうか。
再代入を行わないとルール化することによって、そういった問題を未然に防ぐことが出来ます。
もし下記のようなコードだった場合、間に変換が増えたりしたら書き換え箇所が増えて修正が面倒になってしまいますが、今回の実装例だと配列に関数を追加するだけなので、足すのも引くのも楽に行うことができます。

<?php
$body1 = convert1($body);
$body2 = convert2($body1);
// XXX ここにconvert2_2を追加したい場合は以降の変数名もいくつか調整する必要が出てくる
$body3 = convert3($body2);
$body4 = convert4($body3);

気になる点

速度が犠牲になるのでは?

確かに関数呼び出しが増えるため多少なりとも速度は犠牲になります。
また、phpのバージョンによりますが、7以下であればループ処理もforeachで回した方が早いそうです。
そのためゲームなど速度を重要視する場合は必ずしも今回のような実装にする必要はないと思いますが、大きく差が出ない場合だったり頻繁に書き換えを行うような場合は保守性が高いコードにしておいても損は無いと考えています。

また、phpはバージョンアップによって、配列操作系関数の処理時間も改善されています。
大きく改善したphp8以降がメジャーなバージョンになって行くにつれarray_maparray_reducearray_filter と次にご紹介するアロー関数を利用するのが当たり前になってくるかもしれませんね。

クロージャは冗長で読みにくい

7.4以降のPHPであれば、JSのように短縮記法であるアロー関数が利用可能です!

www.php.net

コメントアウトを削除するコンバート処理であれば下記のように書けますね。

<?php
$_delete_comment_out_func = static::_create_resolver(
    fn (string $body) => preg_replace('/<!--.*?-->/us', '', $body)
);

また、use文も不要になるのでより簡潔な表記が可能です。
しかし欠点もあり、phpは{}で囲って処理のブロックを指定できません。
fn (argument_list) => exprとあるように、式を設定できて、文は設定できないのですね。
つまり今現在では下記のようなコードは使えないということです。

<?php
/// XXX 動作しないコード
fn (array $params) => {
    $result = $f($params);
    return !empty($complate_message)
        ? static::_create_result($params, $result, $complate_message)
        : static::_create_result_update_body_only($params, $result);
};

逆に言えばアロー関数に最適化すれば利用可能になりますので、細かく関数に分けたり、分岐を減らす実装を試してみるのも面白いかもしれませんね。
今回の例ではコンバート処理は全てドメイン層が持ち、アロー関数内でドメインの関数を呼び出すようにすればラップ処理も含めアロー関数化できそうですね。

まとめ

phpに限らずコードを書くときには、分岐や再代入を減らしつつ本当に見て欲しい処理に目が行くように心がけています。
もちろん速度を重視するような場面ではその限りでは無いと思いますが、チーム開発をする上でレビューしやすいコードであれば指摘もしやすく円滑にタスクを進めることができるのではないでしょうか。

GameWithでは一緒に働く仲間を募集しています

GameWithではエンジニアを絶賛募集中です!
サーバーサイドエンジニア、フロントエンジニアだけではなく、Unityエンジニアなども募集中です。
ご興味ある方は是非お気軽にカジュアル面談をお申し込みください!

github.com