判定条件のreturnをどのようにするかについて

Question

下記のような記述で変数に代入している箇所を全てreturnにしたほうが、全体の見通しが良いのでは無いかと指摘されたのですが、
どちらでも変わらないと思うのですが逐次代入するのは邪道でしょうか。
判定ごとにreturnで返却するとどの条件にもマッチしない場合を return ''; のように記載するのが好みでは無いので何か良いご意見は無いでしょうか。
抜けていましたが前提として elseifは利用できない決まりになっています。

   public function get_item_name($item_data) {
       $item_name = '';
       if (isset($item_data['name'])) {
           $item_name = $item_data['name'];
       }
       if (self::is_special($item_data)) {
           $item_name = '特殊名称';
       }
       if (isset($item_data['no']) && $item_data['no'] == '0' ){
           $item_name = '例外名称';
       }
       return $item_name;
   }

Comments

[m.ts10806]

後出しで前提条件が出ているため、回答が多く付いても話が進みません。規約等あるのでしたら質問に記載しておいてください。既に「好みではない」というような質問者さんのコメントがあるため「好みで良い」「自由に回答して良い」という流れになり、本来得たいはずの回答が得られにくい状況になっています。

Answer 1

リーダブルコード的には早めにreturnすべき。
個人の好みなんて曖昧なものは捨てる。

Comments

[kintonet]

ガード節をつくったほうが安牌な気はしています。

Answer 2

$item_data['no'] == '0'
とか残ってる時点で、どうでも良いレビューな気がする。。。

好み問題で言うと、即 return が好き。

Answer 3

邪道じゃないけど無駄な処理をしがちになるかインデントが深くなりがちになる
値が確定したら即返すようにしておくと
無駄な処理をしてしまう心配がなくなるしインデントが深くなりにくい

今のコードでも全てifの条件にかかってしまう$item_dataが渡ってきたら
$item_nameは最終的に必要な代入が１回なのに３回も代入をされる

特にself::is_special()が重い処理であるなら
それが$item_data['no'] == '0'なら無駄になるというのはコスト

Answer 4

好みの問題ですね。
コーディング規約で決まっていないのであれば、現状のままでよいと思います。
規約によってはreturnは関数内で1つというところもありますし、私もこの処理であれば現状のままの方がわかりやすいと思います。

指摘通り各条件でreturnする場合、elseifでつながっていないので、動きが変わってしまいます。
指摘した人はそのことも考慮して指摘しているのでしょうか。
それともelseifになっていないのは、設計・コーディングミスでしょうか。

補足
即return回答が多いので補足しておきます。
私がこのままでよいと回答したのはあくまでもget_item_name関数という閉じた処理だからです。
あれもやってこれもやってみたいな関数では、以降の処理が不要な場合は即returnするほうが私は好きです。
ネストが浅く作れますから、必然的に可読性があがります。

Comments

[kintonet]

elseif自体を許容しない規約のため即時returnしろってことなのかなとは思ってます。

Answer 5

具体的に何をやっているかわからず回答してますけども。

条件が同時に成立せず$item_nameが確定するのであれば、
elseifで数珠つなぎにした方が余計な処理を踏まずに済みます。

   public function get_item_name($item_data) {
       $item_name = '';
       if (isset($item_data['name'])) {
           $item_name = $item_data['name'];
       }
       elseif (self::is_special($item_data)) {
           $item_name = '特殊名称';
       }
       elseif (isset($item_data['no']) && $item_data['no'] == '0' ){
           $item_name = '例外名称';
       }
       return $item_name;
   }

すでに代入した $item_name についてさらに吟味する必要があるなら、
elseifは使えませんが。

Comments

[kintonet]

そのelseifを使えない規約なのと今の所$item_nameを吟味する必要は無いので逆らわずにreturnしておいたほうが安牌かなと思っています。

[退会済みユーザー]

じゃぁ都度returnで。

[SatosiYanagita]

なぜelseifを禁止しているのかわかりませんが、そのような無意味な規約はやめたほうが良いです。
この関数では、関係ないですが、returnする前に何らかのリソースの開放が必要なコードであれば、returnごとにリソースの開放を書かなければなりません。
なお、このルーチンでは、$item_data['name']が入っていて、self::is_special($item_data)が成立したら、'特殊名称'が返ってきます。おそらく期待した値ではないと思います。

[kintonet]

ありがとうございます。elseif自体が可視性が良く無い事は承知しているものの、オープンソースなどを見ても一定数記載があるので規約としてどうだろうかとは思っています。(考えた人は恐らく何かの記述を勘違いして踏襲したんだと思います)

Answer 6

なにか理解がオカシイような・・・
即時returnするならifの順番を逆転させないといけないですよね？
つまり上から順番に条件によって$item_nameを上書きしているのですから
優先順位が高いのは一番下です

Answer 7

「elseifを禁止する」＝「elseifで書くような複数分岐は、途中リターン(や途中ブレイク)などで実現する」という趣旨だと思うので、一択ですね。
１つのメソッドのサイズ（複雑さ）にも制限があるのではないかと思います。

「全体の見通しがよい」とか言うレベルの話ではない。

Answer 8

記載されているコードを前提の話になってしまいますが、

■ 逐次変数に代入し、最後にreturnの場合
⇒関数の最後まで何をしているか読む必要がある。
(該当の分岐処理が完了した後に値が変更されている可能性がある為)

■ 各分岐内でreturnしている場合
⇒returnで処理は終了するため、その後の処理を読む必要がない。

単純に第三者がリーディングした際に掛かるコストを考えるのであれば後者の方が良いかもしれませんね。
まあm6uさんが回答されているとうにelseifでいいかと

Answer 9

要件によります。前後の処理にもよります。
つまり、このコードだけでは何とも言えないのが正直なところです。

このコードだけで判断するなら既に回答にありますがelseifでつないで（elseなしにしました）即returnです。

   public function get_item_name($item_data) {
       if (isset($item_data['name'])) {
           return $item_data['name'];
       }
       if (self::is_special($item_data)) {
           return '特殊名称';
       }
       if (isset($item_data['no']) && $item_data['no'] == '0' ){
           return '例外名称';
       }
       return '';
   }

もし取得先で何かしら判断があるならreturn '';を抜いて
取得先でnull判定するのもありかもしれません。
要は、このメソッドでどこまで肩を持つか 次第ですね。