概要
タイトルの通り、入社してからコードレビューで指摘された基本的なコードの書き方や、実装方法について、自分の戒めとしてまとめたいと思います。
なお、言語はPHPを使用しています。
コードの書き方に関する内容
インデントは揃える
基本中の基ですが、インデントは揃えるようにしましょう。
また、エディタの設定によっては自動フォーマットされる場合もあるため、チームで開発する際は改行やインデントのルールを統一しましょう。
Atomの自動フォーマットの設定に関しては下記のような記事を書いています。
【Atom】エディタの自動フォーマットの設定を統一しよう - Qiita
Docコメントは必ず書く
新たなメソッドの作成や、返すパラメータの変更などの作業をした際は、必ずDocコメントの作成・修正を行いましょう。
/**
* メソッドの説明
*
* @param int $id
* @return bool
*/
function hogehoge($id)
{
return false;
}
マジックナンバーはできるだけ使用せず定数化する
マジックナンバーとは、書いた人にしかわからないようなソースコード上の数値のことです。
例えば、下記のようなif文の条件に使われている、10という数値が、一体何の数値を表しているのか、はじめて見た人にとってわかりませんよね？
表示件数の上限値かもしれないし、どこからか引っ張ってきた数値かもしれない……と、一目で把握することは難しいです。
そのため、マジックナンバーを使用せずに、あらかじめその数値の意味を示した定数を定義し、その定数を使用するようにしましょう。
if ($num < 10) {
return false;
}
// 10って何の数値？？
// 定数化
define('ARTICLE_COUNT', 10);
if ($num < ARTICLE_COUNT) {
return false;
}
1行の文字数が長すぎないようにする
実装していると、ついつい1行の中で配列を取得したり、変換したり、出力したり……というのを同時にやってしまうことがあります。
そうしてしまうと、1行の文字数が長くなり、コードの可読性が悪くなってしまいますので、できるだけ1行でいろんな動作を行わないよう心がけましょう。
PHPに関する内容
使用する変数の初期化
PHPでは、変数を初期化せずに利用することができますが、できるだけ変数の初期化をするようにしましょう。
PHP では変数を初期化する必要はありませんが、そのようにするのはとてもよいことです。 初期化されていない変数の値は、状況に応じたその型のデフォルト値 - boolean なら FALSE、integer や float ならゼロ、 文字列 (echo で使う場合など) なら空の文字列、配列なら空の配列となります。
三項演算子を活用しよう
if ~ else を省略する方法として、三項演算子というのがあります。
例えば、下記の例は、unset_boolがtrueの場合、true\nが出力され、falseの場合はfalse\nが出力されます。
if文を使って書くと長くなる処理を、シンプルに記述できるのが特徴ですね。
echo($unset_bool ? "true\n" : "false\n");
文字列検索ならstrstr()よりもstrpos()
PHPである文字列の中に特定の文字列が含まれているどうかを検証するために、strstr()という関数を使用していました。これは、文字列が最初に現れる位置を見つける関数です。
$email = 'name@example.com';
$domain = strstr($email, '@');
しかし、特定の文字列を探す場合、strstr()ではなくstrpos()の方が高速で処理することができるそうです。コードレビューの際に教えていただいたのですが、知りませんでした！
確かに公式ドキュメントにも書かれています。
注意:
もし特定の haystack に needle があるかどうかを調べるだけの場合、 より高速でメモリ消費も少ない strpos() を代わりに使用してください。
どのくらい速度が違うんだろうと、ふと思ったので調べてみたところ、速度検証している記事がありました。
やっぱりstrpos()が早いみたいですね。
strposとstrstrの速度比較 - 情熱は代行できない。大阪でRSSとiPhoneApp開発のベンチャー社長ログ
preg_replace()に変数を入れるとエラーが出てしまう問題
ある文字列から、正規表現を用いて文字列を検索し、置換を行うためにpreg_match()を使って処理を行っていました。
<?php
$string = 'April 15, 2003';
$pattern = '/(\w+) (\d+), (\d+)/i';
$replacement = '${1}1,$3';
echo preg_replace($pattern, $replacement, $string);
// April1,2003
?>
しかし、正規表現の中に変数名を入れて実行したところ、以下のようなエラーが出てしまいました。
Warning: preg_match() [function.preg-match]: Unknown modifier ‘/’
調べてみると、引数で与えていた変数の中にスラッシュが含まれており、そのスラッシュがエスケープされておらず、エラーが出てしまっているようでした。
同様に、デリミタを{}に変更することで、エラーを解決することができました。
PHPの乱数生成メソッドであるmt_rand()にはバグがある
画像をS3にアップロードするヘルパーの処理を調べていたところ、生成される画像ファイル名の処理にmt_rand()が使用されていました。
このmt_rand()を調べていたところ、実はこのmt_rand()にはバグがあることがわかりました。
そもそも、mt_rand()は乱数生成アルゴリズム「メルセンヌ・ツイスター」を利用して高品質の乱数を生成するメソッドです。
メルセンヌ・ツイスター、通称MTとは、擬似乱数列生成器のひとつです。
https://ja.wikipedia.org/wiki/メルセンヌ・ツイスタ
しかし、実はこのmt_rand()にはバグがあり、このメルセンヌ・ツイスターと挙動が一致していませんでした。
このバグを修正したプルリクエストを送った方がいらっしゃったのですが、マージされたと思いきやリバートされたりと、なにやら一騒動があったようでした。
この騒動の詳細は以下の記事が紹介しています。
PHP の mt_rand() は一貫して壊れている(consistently broken)らしい - 唯物是真 @Scaled_Wurm
この騒動を受けて、mt_rand()の乱数の質を検証した方が記事を公開していました。
検証の結果、本家メルセンヌ・ツイスターと乱数の質はそんなに変わらないことがわかったそうです。
ただ、mt_randが実は奇数しか生成しない、という罠があるらしく、このあたりのバグ？仕様？の調査の重要性を認識しました…。
PHP の壊れた mt_rand の品質を統計的に検証した - iwiwiの日記
ちなみに、PHPの公式ドキュメントを見ると、以下の様な記述がありました。
暗号として使うのには適していないとハッキリ書かれています。注意しましょう。
警告
この関数が生成する値は、暗号学的に安全ではありません。そのため、これを暗号として使ってはいけません。暗号学的に安全な値が必要な場合は、random_int() か random_bytes() あるいは openssl_random_pseudo_bytes() を使いましょう