17
4

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

More than 5 years have passed since last update.

CakePHPAdvent Calendar 2017

Day 25

空かどうかを検査するために empty() を使ってはいけない

Last updated at Posted at 2017-12-24

日頃 CakePHP 関連のリポジトリーに送られてくるプルリクエストを眺めていると、実に多くの方が empty() を安易に使用しているのが目に留まります。実例をご覧ください。

ですが、空かどうかを検査するために empty() を使用するのは適切ではありません。代わりに ! 演算子の使用を推奨します。 !empty() の代わりには、何もせずに真偽コンテキストに渡すか (bool) による型変換が良いでしょう。これは CakePHP のコーディング規約にも書かれています。

empty() は検査対象が未定義であっても E_NOTICE レベルのエラーを発生させません。これは検査したい変数がひょっとすると未定義かもしれない場合には便利ですが、それ以外の場合には変数名の打ち間違いを見落としてしまう原因になりかねません。

もっとも、開発に IDE を利用している方は、自分がそんなミスをするはずがないと信じているかもしれません。あるいは、テストケースをしっかり書いているから大丈夫と安心している方もいらっしゃるかもしれません。

そういった方々を説得するために、この記事では別の問題にも言及します。下のコードをご覧ください。 CakePHP3 の例ですが、ご存知でない方でも問題を理解できると思います。

/** @var \App\Model\Entity\User $user */
if (empty($user->articles)) {
   ...
}

PHP マニュアルの記述を信じるなら、上のコードは以下のコードと同じことです。

/** @var \App\Model\Entity\User $user */
if (!isset($user->articles) || $user->articles == false) {
   ...
}

さて、この時 CakePHP の内部ではどんな処理が行われるのでしょうか。

CakePHP3 では Entity のプロパティーはマジックメソッドによって実装されています。このため、式 !isset($user->articles)__isset() メソッドを呼び出します。この式の結果が偽になると(アソシエーションがあればそうなるでしょう)、次の式 $user->articles == false が評価されることになり、今度は __get() メソッドが呼び出されます。

Entity の __isset() メソッドは内部的に has() メソッドを呼び出しており、 has() メソッドは内部的に get() メソッドを呼び出しますが、この get() メソッドというのは __get() メソッドが内部的に呼び出すメソッドです。結果として、一度の empty() で二度も get() メソッドが呼び出されてしまうわけですから、非効率なのがお分かりいただけるだろうと思います。

empty() を使わない例と比較してみましょう。

/** @var \App\Model\Entity\User $user */
if (!$user->articles) {
   ...
}

こちらは __get() メソッドが呼び出されるだけですね。また、 Entity は ArrayAccess を実装していますが、配列形式のアクセスでも同じことが言えます。 empty()offsetExists()offsetGet() を呼び出します。

それにしても、どうして empty() が安易に使用されてしまうのでしょう。私が知る限りでは Scrutinizer というコード解析サービスが empty() の使用を推奨しているようですが、もしかするとそうした影響があるのでしょうか。

ちなみに Scrutinizer が empty() の使用を推奨しているのは、その empty という名称が持つ可読性のためだそうです。もしそれだけの理由であればやめた方が良いでしょう。私には、むしろ可読性を落としているようにさえ思えます。

なぜなら empty($var) という記述は、ひょっとすると $var が未定義になる場合があるかもしれない、という誤った情報を読み手に伝えてしまうからです。スコープ内に $var が宣言されていればそんな誤解も早く解けますが、それがテンプレートのような被インクルードファイル中でのことだったらどうでしょう。当然、読み手は、変数が未定義になる場合があるのかもしれない、と疑わなければなりません。

!$var という記述になっていれば、最初からそうした疑いを読み手に抱かせないはずです。これこそが可読性なのではないでしょうか。

もし CakePHP 関連のリポジトリーにプルリクエストを送る機会がありましたら、ご自身の行った変更の中に不適切な empty() の使用が含まれていないかを事前にご確認ください。また、既存のコードに含まれている不適切な empty() の使用を修正するプルリクエストも歓迎します。その際にはどうぞプルリクエストが大きくなりすぎないようにもご配慮くださいませ。

17
4
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
17
4

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?