current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free.
up vote 2 down vote favorite

I have a codeigniter app and in my model, I always return true or false for all functions, and if I have data that needs to be passed, I also set a property that contains my data.

The only trouble is, in my controller, if I have to call 3 or 4 methods in my model, the code gets really repetitive.

If ( $this->my_model->functionA() ) 
{
    $localvar = $this->my_model->data();
}
else
{
    show_error("Error A");
}
If ( $this->my_model->functionB() ) 
{
    $localvar = $this->my_model->data();
}
else
{
    show_error("Error B");
}
If ( $this->my_model->functionC() ) 
{
    $localvar = $this->my_model->data();
}
else
{
    show_error("Error C");
}

I'm wondering if i change the logic so that the functions don't return true, but return the data instead... does it simplify things alot? I think I'd still need code like this: 

If (! $this->my_model->functionA() ) 
{
    show_error("Error A");        
}
else
{
    $localvar = $this->my_model->data();
}

Or is there a way I can combine my $localvar assignment statement with the if statement?

Is there a better way to do this?

share|improve this question
    
By "error" do you mean system error or user error? –  Matt S Jan 9 '13 at 19:48
    
in this case, if the method in the model fails, it's because it can't connect to a device - aka. a system error –  dot Jan 9 '13 at 20:05

1 Answer 1

up vote 1 down vote accepted

Since you're checking for system errors, I would use exceptions instead of just return values. And keep in mind your model preserves state, so its data does not need to be returned for every function. This is far more readable:

try {
    $this->my_model->functionA();
    $this->my_model->functionB();
    $this->my_model->functionC();
    $localvar = $this->my_model->data();
}
catch (Exception $e) {
    show_error($e->getMessage());
}
share|improve this answer
1  
Using exceptions for this is good practice, but you should catch in a more specific way. As it is written in your example, you might accidentally leak internal program information, e.g. when an SQL query throws an exceptions, the user might get to see parts of the SQL you're issuing, which is a security issue. An approach that works well is to define a base class for user-visible exceptions, and derive all exceptions that may be shown to the user from it; then you can catch on that base class first, displaying the message, and then on Exception, showing a generic error message. –  tdammers Jan 9 '13 at 20:41
    
@tdammers - do you have an example of this that I can refer to somewhere? –  dot Jan 10 '13 at 15:39
    
@Matt, this means that in my model i would need to use the "throw new Exception()" method, correct?, and also make sure then that in the model method, i don't already have a try catch() that would "interfere" with the try / catch in the controller. Or could I just throw it again from the catch in the model? –  dot Jan 10 '13 at 15:40
1  
@dot That's right. Your model might try/catch for some other reason, but it should throw a specific exception if there is a system error. –  Matt S Jan 10 '13 at 15:44
    
@Matt. Thanks for the clear & concise advice. –  dot Jan 10 '13 at 16:00

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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