I have a controller that does:
$objImporter->import('filename');
Then a class that:
public function import ($filename){
if(this->notExcelFile){
return "not an excel file";
}
else carry on
}
Instead of having lots of returns (I only like one exit from a function, at the bottom) I usually throw and exception and then return that in the catch.
Is this the best way to do this? I started life as a VB developer and wonder if there is a better OO way to do this?
Mick
A good practice is to check is it's Excel file and return data or link to data if it's Excel. If it's not Excel file, you should return false
(or return -1
in some cases), for example.
Then in controller you're just checking data, like:
if($data){
// It's not Excel file, there is not data
}else{
// work with data
}
Add try catch block to your controller:
try {
$objImporter->import('filename');
} catch (Exception $e) {
//Do something with error. You can send mail alert.
}
Simple throw exception if any error occurs.
public function import ($filename) {
if(this->notExcelFile){
throw new Exception('Given file is not an Excel file');
} else {
//Your logic goes here
}
}
The easy answer is, throw an Exception. This is quite popular with Java/PHP codebases these days. But in reality, using Exceptions for control flow breaks OO design. Messages are sent between objects, but because of the use of Try/catch your code starts jumping up and down with an irregular and impossible to follow control flow.
Being more purist, in an OO ideal world, what you would return would be some sort of Result object that would contain whether the operation succeeded and a reason if it didn't. Something like:
public function import ($filename){
if (this->notExcelFile) {
return new UnsuccessfulResult("not an excel file");
}
else {
//carry on
return new SuccessfulResult($excelFile);
}
}
The caller would work something like:
$result = $objImporter->import('filename');
if(!$result->succeded()) {
//show error to the user
}
// continue with a successful result
$exelFile = $result->getResult();
How would you go about implementing something like this?
You could create an interface such as ResultLike
interface ResultLike {
/** @return boolean */
public function succeeded();
/** @return mixed */
public function getResult();
}
Which you could then implement in your objects:
class UnsuccessfulResult implements ResultLike {
public function __construct($message) {
$this->message = $message;
}
public function succeeded() {
return false;
}
/** @return mixed */
public function getResult() {
return $this->message;
}
}
class SuccessfulResult implements ResultLike {
public function __construct($result) {
$this->result = $result;
}
public function succeeded() {
return true;
}
/** @return mixed */
public function getResult() {
return $this->result;
}
}
Happy days
Exceptions are just fine and much better than returning a fake result like "null" or "not an excel file".
More "OOP-ish" approach is to use the Null object pattern.
In the code you can have something like this:
public function import ($filename) {
if(this->notExcelFile){
return new NullExcelFile();
} else {
//read the data
return new ExcelFile($data);
}
}
Let's imagine you have an API method wich returns excel file content as json:
$excel = import('path/to/the/file');
$jsonData = $excel->asJson();
echo json_encode($jsonData);
Now you have no worries about whether the file existed or not, now "null" handling, no exception handling, the code just works the same way regardless of whether file existed or not.
Of course, the returned response will be different, the real ExcelFile
will return the json with file content, and the NullExcelFile
will return json with error, for example {"error": "no_file", "message": "The specified file does not exist"}
. And the client-side code will handle it and render the error message for the user.