2018-01-22

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

引数が多いメソッドを見るとモヤモヤします
個人的には引数に関しては基底クラスのメソッドは4つまで、それ以外は3つまでと定めていますｰ 4つの原点はPerl Best Practiceだったのではないかと記憶しています
また、関数内で登場する変数も同様の数に抑えるべきでしょう

何がよくないか？

パラメータの数が多いということは当該メソッドの複雑さをそのままに表します
少ないパラメータ(複雑な連想配列はこの限りではありません)はメソッドの単純さを表します
ネストレベルが4以上の連想配列やオブジェクトのメソッド呼び出しは何らかの危険信号です

どうすればよかったか？

引数を4つ以上受け取るメソッドは一つのことをうまくやれないか検討しましょう
また、ローカル(レキシカル)変数が4つ以上出現する関数は登場人物が多すぎるので、その分パラメータを減らしましょう
パラメータとローカル(レキシカル変数合わせて6つが初見の限界だと思います)

伝えたいこと

パラメータの数は少なくする
まとめられるものでない引数が5つ以上あるなら、そのメソッドは分解すべき
メソッド内でのローカル(レキシカル)変数が多い場合も注意、その際は引数を少なくすべき
単純に保つには引数の数を抑え、引数が増えるのであればローカル(レキシカル)変数をその分、少なくしましょう

2018-01-11

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

こういうコードを見ると不安になります
PHPでかつ、Laravelであると想定します

$foo = SomeModel::select("select * from some_table where some_column = {$condition}");

何がよくないのか？

$orderにインジェクション可能なSQLがユーザ入力で指定された場合に大切なデータが全て消え去ります
特にテーブル名をそのままレスポンスに含めていると、簡単にDELETE文の対象にされてしまいます

どうすればよかったか？

プレースホルダを必ず用いる様にしましょう
プレースホルダを用いることが出来ない場合は少なくともSQLと想定される文字列をエスケープできる様にしましょう
上記の例であれば下記の様なコードに置き換えるだけでインジェクションの脅威はなくなります

$foo = SomeModel::select('select * from some_table where some_column = ?'. [$condition]);

伝えたいこと

SQLを発行するコードを際は常にSQLインジェクションが発生しないコードを書きましょう
最もコストが低く、簡単にインジェクションを許さない方法はプレースホルダを検討することです
また、プレースホルダが利用できないのであれば最低でもSQLとみなされそうな文字列はエスケープしましょう
テーブル名をレスポンスに含めている場合は攻撃者に攻撃するための情報を与えている様なものです
簡単に攻撃を許さない様に細心の注意を払いましょう

2018-01-07

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

間が空いてしまいましたがこの記事です
ただし、これって何でこう思ったんだっけ？という裏をとれなかった記事です
正直、ちょっとモヤモヤしつつ書いています。。

何がよくないのか？

3度もコピペされるコードは共通化、もしくは抽象化されるべき
DRY(Don't repeat yourself)の概念です
ですが、YAGNI(You ain't gonna need it)の原則に則って、最初からコピペに対抗することは避けたいです
2つまでは何とかなります。3つ以上のコピペは保守性が圧倒的に下がるので避けましょう

どうすればよかったか？

意味論で同じものを求めるのであれば抽象化しましょう
逆に日付の制御であったり、ファイルを管理するといったものはそれに適した名前を持つクラス/関数を用意しましょう
同じコードを必要とするのであれば、意味論で継承関係を築くことが容易です
意味的に全く異なることを行っているコードを無理に継承関係に落とす必要はありません
こういう場合はstaticな関数を用意して挙げましょう

伝えたいこと

コピペは2度目まで、もしくはプロジェクトでN度目までと定めましょう
理由としては誰かが書いたコードをコピペしたことであなたがそのコードを保守する責務を負わないため
また、そのコードにバグがあった際にコピペ元のすべてのコードがあなたの責任にならない様にするためです
そして、共通化でなく抽象化出来るかを先に検討しましょう
抽象化出来るのであれば差分プログラミングが可能となります。そして、似たようなコードは少しの違いを持っています
レイヤを適切に整えて、少しの差分だけをコードで表現出来る様にしましょう

2018-01-04

Wikiに書こう

今回はレビューとは関係ない話になります
ふと、チケットを見ていると調査内容をExcelにまとめている方がいて少し驚いたので書いてみます

何がよくないのか？

Excelは基本的に有料ソフトです
また、立ち上がりまでに多少の時間を要求します
ダウンロード時間や、不要なディスク容量を使うことも挙げられます
不要になったファイルを消す努力も挙げられるかも知れません
そして、Excelは階層構造を表現するためにはどちらかというと不便なツールです

どうすればよかったか？

Wiki記法で書けるSaaSをあなたの職場が採用しているならそれを使いましょう
大抵のWikiはハイパーリンクを使えるので、関連するページがあればいくらでも参照をつけることができます
また、数十年前とは異なり一般的なブラウザは無料です
そして、多くのWikiはシンタクスハイライトをサポートしています
これはExcelにテキストを単なるテキストを貼り付けるよりも遥かに可読性が増します
シンタクスハイライトをExcel上で表現したいですか？私は嫌です。

伝えたいこと

Excelはよいソフトウェアかも知れませんが、調査結果のログとしてはアップデートや俯瞰するという面で不適です
更にExcelを起動するためにマシンパワーを必要とします(これはブラウザのタブを一つ増やすよりもっとです！)
そして、Excelは基本的に有料です
一方でWikiであれば、ブラウザさえあれば誰でも(別途、権限が必要かも知れませんが)閲覧が可能です
そして、Wikiは俯瞰することを目的とし、構造化を図ることを可能とします
何よりも多くのWikiはシンタクスハイライトを行いコードの可読性に努めます
今、Excelに書いていることをWikiに書くことで関連性を考える材料としてはどうでしょうか？
もう一つ挙げるとすれば、Wikiでリンクが貼られているならば、探すことが容易になうことです

2018-01-03

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

調査中に下記の様なコードを見るとツライ気持ちになったりします

array(2) {
  [0]=>
  array(1) {
    ["foo"]=>
    int(1)
  }
  [1]=>
  object(stdClass)#1 (1) {
    ["foo"]=>
    int(2)
  }
}

何がよくないのか

同様の値を保持している様に見えますが、配列とオブジェクトが格納されています
ループで扱おうとした際に配列であるかオブジェクトであるかを見るという処理が行われるケースも稀に見かけます
これが連想配列で適切なキー名が与えられているのであれば(おそらく)問題ありません
PHPが配列と連想配列を同じシンタクスで使えることや、配列を途中で連想配列に出来てしまうあたりも弊害として挙げられます
配列を用意する側は利用する側に比べて多岐に渡るため、不要な処理を要求しない様に実装されるべきです

どうすればよかったか？

格納時に配列にキャストしてしまう
より正しい対応方法は格納前の取得方法を見直すことですね
ジェネリクスを使える言語であれば、格納内容を指定することでこれを回避できます
ただし、ジェネリクスを使える言語でもObjectを指定しているケースは危険な状態です

伝えたいこと

配列に型が違うものを格納しない
これは読み込む処理の実装が不要に複雑になってしまうからである
ジェネリクスを使える場合は正しく型を指定しましょう

2018-01-02

計算量を常に意識しよう

こういうコードを見るとパフォーマンスが劣化しないか気になります

foreach ($foo => $bar) {
    if (!in_array($bar, $some_list)) {
        continue;
    }

    // 何かの処理
}

何がよくないのか？

in_array は毎回、配列内を検索します
今回はPHPのコードを追ってみましょう
まずはコードの定義された箇所を特定します

$ grep -r in_array php-src | grep PHP_FUNCTION
php-src/ext/standard/array.c:PHP_FUNCTION(in_array)
php-src/ext/standard/php_array.h:PHP_FUNCTION(in_array);

ext/standard/array.cを見ればよいことがわかります

PHP_FUNCTION(in_array)
{
    php_search_array(INTERNAL_FUNCTION_PARAM_PASSTHRU, 0);
}

php_search_arrayの定義がどこにあるか調べます

$ grep -lr php_search_array php-src
php-src/ext/standard/array.c

関数を確認するとstrictモード|数値|文字列|それ以外で処理が分かれています
常にZEND_HASH_FOREACH_KEY_VALが呼ばれることがわかります
下記のコードは個別の内容を折り畳んだもの

    if (strict) {
        ZEND_HASH_FOREACH_KEY_VAL(Z_ARRVAL_P(array), num_idx, str_idx, entry) {...} ZEND_HASH_FOREACH_END();
    } else {
        if (Z_TYPE_P(value) == IS_LONG) {
            ZEND_HASH_FOREACH_KEY_VAL(Z_ARRVAL_P(array), num_idx, str_idx, entry) {...} ZEND_HASH_FOREACH_END();
        } else if (Z_TYPE_P(value) == IS_STRING) {
            ZEND_HASH_FOREACH_KEY_VAL(Z_ARRVAL_P(array), num_idx, str_idx, entry) {...} ZEND_HASH_FOREACH_END();
        } else {
            ZEND_HASH_FOREACH_KEY_VAL(Z_ARRVAL_P(array), num_idx, str_idx, entry) {...} ZEND_HASH_FOREACH_END();
        }
    }

次はZEND_HASH_FOREACH_KEY_VALを調べれば良いことがわかります
php-src/Zend/zend_hash.cがあるのでそちらを読みましょう

$ grep -wr ZEND_HASH_FOREACH_KEY_VAL php-src | grep define
php-src/Zend/zend_hash.h:#define ZEND_HASH_FOREACH_KEY_VAL(ht, _h, _key, _val) \

内部で与えられた配列の要素が以下の条件を満たすまで無限ループを行っていることがわかります
目的の値が見つかる
配列の要素が見つかった

convert:
    {
        HashTable *new_ht = zend_new_array(zend_hash_num_elements(ht));

        ZEND_HASH_FOREACH_KEY_VAL(ht, num_key, str_key, zv) {
            do {
                if (Z_OPT_REFCOUNTED_P(zv)) {
                    if (Z_ISREF_P(zv) && Z_REFCOUNT_P(zv) == 1) {
                        zv = Z_REFVAL_P(zv);
                        if (!Z_OPT_REFCOUNTED_P(zv)) {
                            break;
                        }
                    }
                    Z_ADDREF_P(zv);
                }
            } while (0);
            /* Again, thank ArrayObject for `!str_key ||`. */
            if (!str_key || ZEND_HANDLE_NUMERIC(str_key, num_key)) {
                zend_hash_index_update(new_ht, num_key, zv);
            } else {
                zend_hash_update(new_ht, str_key, zv);
            }
        } ZEND_HASH_FOREACH_END();

        return new_ht;
    }
}

上記からわかることは2つあります
PHPのin_arrayは毎回ループを行う
配列後方に目的の値があるほど探索が遅くなる

どうすればよかったのか？

最初にarray_flipした配列を用意してissetを使いましょう

$flipped = array_flip($some_list);
foreach ($foo => $bar) {
    if (!isset($flipped[$bar])) {
        continue;
    }

    // 何かの処理
}

`array_flip`した内容を`isset`で見る方が本当に高速か検証する

ベンチマークをとります

<?php
require_once '.composer/vendor/devster/ubench/src/Ubench.php';

$list = range(1, 10000);

$b = new Ubench();
$b->start();
foreach (range(1, 10000) as $i) {
    in_array(1, $list);
}
$b->end();
echo 'in_array_1st: ' . $b->getTime(), "\n";

$b->start();
foreach (range(1, 10000) as $j) {
    in_array(10000, $list);
}
$b->end();
echo 'in_array_last: ' . $b->getTime(), "\n";

$b->start();
$flipped = array_flip($list);
foreach (range(1, 10000) as $l) {
    isset($flipped[1]);
}
$b->end();
echo 'isset_1st: : ' . $b->getTime(), "\n";

$b->start();
$flipped = array_flip($list);
foreach (range(1, 10000) as $m) {
    isset($flipped[10000]);
}
$b->end();
echo 'isset_last: ' . $b->getTime(), "\n";

ベンチ結果はこの様になりました
期待した通り、末尾に目的の値がある場合に劣化します
逆にissetの場合は劣化することもなく、先頭要素を探すのに比べても若干高速ですね

$ php test.php
in_array_1st: 6ms
in_array_last: 1.680s
isset_1st: : 5ms
isset_last: 5ms

伝えたいこと

(特に組込)関数が何をしているのかを理解して使う様にしましょう
内部でループが行われている場合は、いかにループを少なくするかを検討しましょう

2018-01-01

安易なgetter/setterを作らない

明けましておめでとうございます
こういうコードを見るとモヤモヤします

if ($foo->getDate() > $today) {
     return false;
}

何が良くないのか？

$foo->getDate() を使って条件判定を行っている箇所が正しくカプセル化を行えていないと感じます
また、上記が何の日付を意味しているのか全くわからない点にも問題を感じます
Why getter and setter methods are evilで書かれていたことですね

どうすればよかったか？

以前にQiitaでも書きましたが、モデルにデータを閉じ込めその振る舞いとしてカプセル化しましょう
そして、その条件自体に正しく本質的な名前を付けてあげましょう
最初の例が仮にブログ記事の公開であるとします
であれば、インスタンス名は$articleとなるでしょう
そして、getDateはisPublishedといった名前になるはずです
$todayをパラメータとして渡すべきかどうかは使っているDate系のライブラリによっても変わるでしょう
ここではDateインスタンスの生成にコストがかかるものを想定してパラメータとして渡しています

if ($article->isPublished($today)) {
    return false
}

伝えたいこと

getter/setterをとりあえずやなんとなくで用意しない
getterやsetterを用いてすべきことを補完したメソッドを用意してそれらを使う様にする
補完したメソッドが適切な名前を与えられていれば、自然言語に近づき何をするメソッドであるかもわかりやすくなります

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

何がよくないか？

どうすればよかったか？

伝えたいこと

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

何がよくないのか？

どうすればよかったか？

伝えたいこと

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

何がよくないのか？

どうすればよかったか？

伝えたいこと

Wikiに書こう

何がよくないのか？

どうすればよかったか？

伝えたいこと

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

何がよくないのか

どうすればよかったか？

伝えたいこと

計算量を常に意識しよう

何がよくないのか？

どうすればよかったのか？

array_flipした内容をissetで見る方が本当に高速か検証する

伝えたいこと

安易なgetter/setterを作らない

何が良くないのか？

どうすればよかったか？

伝えたいこと

`array_flip`した内容を`isset`で見る方が本当に高速か検証する