2
\$\begingroup\$

Lately, I'm using domain model pattern, and my controllers methods often looks like (yii2 example, verify user in loyalty system with Ajax response).

if (LoyaltyDomModel::loyaltyVerification($post_data)) {

    if (LoyaltyDomModel::setVerify(Yii::$app->user->identity, $post_data)) {

        return  ['body'=>'successfull','status' => true];
    }
    else {
        return  ['body'=>'unable update user','status' => 'error'];
    }
}
else {
    return  ['body'=>'errorcardno','status' => 'error'];
}

If logic is more complex I have a lot of if statement with model methods which return bool values, for example (I'm trying to decouple models logic in atomic methods), and it looks like not healthy. Should I encapsulate logic of return errors in models? What is best practices?

\$\endgroup\$
  • 3
    \$\begingroup\$ Welcome to Code Review. "Too many if statements" is one of our most common inappropriate question titles. Our convention is to state what the code accomplishes, and show the code in context — preferably your entire function, for something like this. See How to Ask. \$\endgroup\$ – 200_success Mar 1 '17 at 15:20
  • \$\begingroup\$ ok, I'm edit my question \$\endgroup\$ – bxN5 Mar 1 '17 at 15:34
  • \$\begingroup\$ IMO, I'd push those if statements into a trait class and call them individualistically during the controllers execution. \$\endgroup\$ – David J Eddy Mar 1 '17 at 15:43
4
\$\begingroup\$

I advice you forego if checks completely and go for try-catch blocks, and have your verifiers throw exceptions on error. So, you do something like this:

try {
     LoyaltyDomModel::loyaltyVerification($post_data);
     LoyaltyDomModel::setVerify(Yii::$app->user->identity, $post_data);

     return  ['body'=>'successfull','status' => true];
} catch (LoyaltyVerificationException $e) {
     return ['body' => $e->getMessage(), 'status' => 'error']
} catch (UserUpdateException $e) {
     return ['body' => $e->getMessage(), 'fields' => $e->getFields(), /* you can pass custom info to exceptions, right? */, 'status' => 'error'];
}

Also, it's not a good idea to return bool in one case ('status' = true) and string in another ('status' = 'error');

\$\endgroup\$
  • \$\begingroup\$ A generally good suggestion, but I would be somewhat concerned about mixing expected application events and code paths (like a user verification process failure, input data validation, login verification, etc.) with those that truly represent some unexpected exceptional state in your application (like failing to get a database connection). In this case it is unclear from OP whether these else conditions truly represent application state failures or validation-type failures which would be expected in normal application execution. \$\endgroup\$ – Mike Brant Mar 1 '17 at 19:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.