2010-11-22 14 views
4

これはかなり規則的に起こっている問題で、ベストプラクティスの状況を見つけたことはありません。例外はおそらく私が働いているアプリケーションは使用しないので、現在使用されているメソッドに固執しようとしています。複数のif/elseifとエラーメッセージング/エラー処理のベストプラクティス

3つ、4つ、5つ以上の異なる条件をチェックする必要があり、エラーメッセージが設定されているか処理が続行されている場合に、ステートメント、リターン、メッセージなどをレイアウトする最良の方法は何ですか?コードの先頭ですべてのエラーチェックを物理的に行うのがベストプラクティスですか?

実際のタイプの条件の例を示します。

function process($objectId,$userId,$newData) 
{ 
    $error = ''; 
    if(($object = $this->getObject($objectId)) && $object->userOwnsObject($userId)) 
    { 
     if($this->isValid($newData)) 
     { 
      if($object->isWriteable()) 
      { 
       if($object->write($newData)) 
       { 
        // No error. Success! 
       } 
       else 
       { 
        $error = 'Unable to write object'; 
       } 
      } 
      else 
      { 
       $error = 'Object not writeable'; 
      } 
     } 
     else 
     { 
      $error = 'Data invalid'; 
     } 
    } 
    else 
    { 
     $error = 'Object invalid'; 
    } 
    return $error; 
} 

OR

function process($objectId,$userId,$newData) 
{ 
    $error = ''; 
    if((!$object = $this->getObject($objectId)) && !$object->userOwnsObject($userId)) 
    { 
     $error = 'Object invalid'; 
    } 
    elseif(!$this->isValid($newData)) 
    { 
     $error = 'Data invalid'; 
    } 
    elseif(!$object->isWriteable()) 
    { 
     $error = 'Object not writeable'; 
    } 
    elseif(!$object->write($newData)) 
    { 
     $error = 'Unable to write to object'; 
    } 
    else 
    { 
     // Success! 
    } 
    return $error; 
} 

それは、この場合には、オプション2を移動するための方法であると私には明らかです。はるかに明確です。

function process($objectId,$userId,$newData) 
{ 
    $error = ''; 
    if(($object = $this->getObject($objectId)) && $object->userOwnsObject($userId)) 
    { 
     $this->setValidationRules(); 
     $parent = $object->getParentObject(); 
     $parent->prepareForChildUpdate(); 

     if($this->isValid($newData,$parent)) 
     { 
      $newData = $this->preProcessData($newData); 

      if($object->isWriteable()) 
      { 
       // doServerIntensiveProcess() has no return value and must be done between these two steps 
       $this->doServerIntensiveProcess(); 

       if($object->write($newData)) 
       { 
        // No error. Success! 
        $parent->childUpdated(); 
       } 
       else 
       { 
        $error = 'Unable to write object'; 
       } 
      } 
      else 
      { 
       $error = 'Object not writeable'; 
      } 
     } 
     else 
     { 
      $error = 'Data invalid'; 
     } 
    } 
    else 
    { 
     $error = 'Object invalid'; 
    } 
    return $error; 
} 

をORそれでいくつかの問題を持っている、これは

function process($objectId,$userId,$newData) 
{ 
    $error = ''; 
    if((!$object = $this->getObject($objectId)) && !$object->userOwnsObject($userId)) 
    { 
     $error = 'Object invalid'; 
    } 
    // Is it wrong to hate multi-line conditionals? 
    elseif(!$this->setValidationRules() || (!$parent = $object->getParentObject()) || 
     !$parent->prepareForChildUpdate() || !$this->isValid($newData,$parent)) 
    { 
     $error = 'Data invalid'; 
    } 
    elseif((!$newData = $this->preProcessData($newData)) || !$object->isWriteable()) 
    { 
     $error = 'Object not writeable'; 
    } 
    // Where does doServerIntensiveProcess() with no return value go?? 
    elseif(!$object->write($newData)) 
    { 
     $error = 'Unable to write to object'; 
    } 
    else 
    { 
     // Success! 
     $parent->childUpdated(); 
    } 
    return $error; 
} 

私はちょうどこのネストされた場合は、これを処理するための最良の方法のわからない:今、我々はそれが少し複雑にすることができますそれでは、そうしていれば、そのような種類の機能です。あなたが提供できる何らかの洞察力を知らせてくれてありがとう!

+0

あなたは例外があることを知っているようですが、なぜそれらを使用したくないのですか?私は彼らがあなたの現在のコードベースで使われていないと理解していますが、今は始めるのが良い時期です。エラーを表示するために戻り値の文字列を使用することは、絶対にひどい考えです。 – ryeguy

+0

ええ、そうです。特定の方法でスタイルされた大規模なコードベースです。私はつま先を踏みつけたり、他の開発者を例外の謎を探索する際に庭の道を上って行きたいとは思わない。 – fred

答えて

4

はそうのようなものです:

function process($objectId,$userId,$newData) 
{ 
    $object = $this->getObject($objectId); 

    if($object === false) 
    { 
     return "message"; 
    } 

    if($object->userOwnsObject($userId) === false) 
    { 
     return "message"; 
    } 

    if($this->setValidationRules() === false) 
    { 
     return "unable to set validation rules"; 
    } 

    if(false !== ($parent = $object->getParentObject())) 
    { 
     return "unable to get parent object"; 
    } 

    /*... etc ...*/ 

    //if your here the all the checks above passed. 
} 

それをあなたにもリソースを節約するこの方法を実行しては、あなたの場所に直接戻ってきて、コードはきれいに見え、2つの多くの巣の必要はありません

あなたのビルドあなたは新しいコードでで例外を使用傾ける私はなぜ表示されないゼロから機能をINGの、それは現在のアプリケーションに干渉し、そしてライブ

function process($objectId,$userId,$newData) 
{  
    if(false !== ($parent = $object->getParentObject())) 
    { 
      throw Exception("unable to get parent object"); 
    } 

    /*... etc ...*/ 
} 

try 
{ 
    $this->process(....); 
} 
catch(Exception $e) 
{ 
    show_error_page('invalid.php',$e); 
} 

単純かになりません。別の方法は、あなたが行うことができますので、上記の代わりにshow_error_pageののよう

abstract class Error 
{ 
    public static InternalError(Exception $Ex) 
    { 
     Logger::LogException($Ex); 
     //Then flush all buffers and show internal error, 
    } 
} 

ようInternalErrorと呼ばれる静的メソッドでエラー処理クラスを作成することです:

try 
{ 
    $this->process(....); 
} 
catch(Exception $e) 
{ 
    Error::InternalError($e); //this provides user with an interface to report the error that has just been logged. 
} 

こうしてすべてあなたのException(複数可)ログに記録され、あなたがより速くエラーを追跡していない目に見えてエラーを確認するためにメンバーに依存しているが、電子メールでの素敵な謝罪を得ることができることを意味し、あなたの管理システム内で見ることができます彼らが何をしようとしているかを記述するように要求するフォームでは、フォームにエラーIDが添付されるため、ユーザーはエラーを追跡できます。

これは、エラー処理IMOの最良の方法です。

+0

詳細な対応をありがとうございます。私は複数の戻り値とエラーログにあなたに同意します。私はそれが下になると思う私は私の知る限りではなく、このアプリ内の規範ではないことを行う必要があります。 – fred

+0

アプリケーションの作業量はどれくらいですか? – RobertPitt

+0

私はラインワイズではわかりません。私は約6から8メガバイトのテキストだと思う。 – fred

0

これはどうですか?より複雑な例については

function process($objectId,$userId,$newData) 
{ 
    if((!$object = $this->getObject($objectId)) && !$object->userOwnsObject($userId)) 
     return 'Object invalid'; 
    elseif(!$this->isValid($newData)) 
     return 'Data invalid'; 
    elseif(!$object->isWriteable()) 
     return 'Object not writeable'; 
    elseif(!$object->write($newData)) 
     return 'Unable to write to object'; 

    // Success! 
} 

::私はクリーンなコードを維持するために行う傾向にある何

function process($objectId,$userId,$newData) 
{ 
    if(!($object = $this->getObject($objectId)) || !$object->userOwnsObject($userId)) 
     return 'Object invalid'; 

    $this->setValidationRules(); 
    $parent = $object->getParentObject(); 
    $parent->prepareForChildUpdate(); 

    if(!$this->isValid($newData,$parent)) 
     return 'Data Invalid'; 

    $newData = $this->preProcessData($newData); 

    if(!$object->isWriteable()) 
     return 'Object not writable'; 

    // doServerIntensiveProcess() has no return value and must be done between these two steps 
    $this->doServerIntensiveProcess(); 

    if(!$object->write($newData)) 
     return 'Unable to write object'; 

    // No error. Success! 
    $parent->childUpdated(); 
    return ''; 
} 
+1

地獄への道は、括弧をオプションとして扱うコードで舗装されています。ちょうどFYI。 –

+0

私は怠け者でした。この場合、単なる例であるので、実際には問題ではありません。 – ThiefMaster

0

私は完全に

if($object->isWriteable()) 
{ 
    if($object->write($newData)) 
    { 
     // .. 
    } 
} 

を省略して、オブジェクトライト()(戻り値なし)の代わりを呼び出すときに例外をスローしていました。他の例

if ($something->isValid($data)) { 
    $op->doSomething($data); // throws XyException on error 
} 

ための同じあなたは本当にこのような構築物を使用する場合は、あなたもSWTCH -statement

switch (true) { 
    case !$something->isValid($data): 
     $errors = "error"; 
     break; 
    // and so an 
} 

を使用することができますしかし、私は本当に例外をお勧めします。

0

複数行の条件文を書くのは間違っているとは思っていませんが、条件を変数に入れてif文で使用すると読みやすくなります。私はelseifの構成が良いと思います。

関連する問題