Question

I'm writing some classes and would like to know how to properly handle scenarios which are not success. For example, a file upload class, which as a parameter accepts the name of $_FILES resource:

class FileUpload {
    private $file;

    public function __construct($file) {
        $this->file = $file;
    }

    public function upload() {
        if (!isset($_FILES[$this->file])) { 
            throw new Exception("Reference to non existent resource.");
        }

        if ($_FILES[$this->file]['error'] !== 0) { 
            throw new Exception("Resource indicates error.");
        }

                // All other sorts of checks here, like security checks,
                // and then finally moving of the file to it's final destination
    }
}

Is this proper form? My line of thinking is this: when a developer creates a new instance of FileUpload, his intention is either have the file uploaded, or know why it wasn't uploaded, and this class will give him just that. Either:

1) true (file was tested for proper format, security checked, maybe even further processed if it was an image, and now it is exactly where you want it to be. In short - everything you wanted worked out)

2) Exception (Something went wrong, and the message is letting you know exactly what, so you can deal with it.)

IF all this is fine, should I just use Exception, with custom messages, or create custom exceptions for everything, for example WrongMimeType?

If it matters - classes would be open source, available for everyone to use, so from that aspect too I'd like to make them as standardized, developer-friendly and easily plugged into existing software as possible.

Was it helpful?

Solution

This should probably be moved to codereview because it looks to me like a question about coding style.

However:

Your upload() method is badly designed. It should accept a parameter, not grab the data from $_FILES - the outside user should pass it into the function. No class should generally grab data from a superglobal or global variable.

Honouring that dismisses the first reason to throw an exception. :)

And then? It's kind of hard to discuss details further because there isn't really much code to see, so I fall back to the general remarks:

Exceptions should NOT be used as a replacement for goto. They should signal exceptional state that could not be anticipated.

When validating a form, invalid form data is no exceptional state that cannot be anticipated - it is one of two usual cases. Effectively you are validating the form data in your class - you shouldn't throw exceptions if the form is not valid.

Another thing: Do not hide the thing that went wrong in the exception message. It makes it so much harder to use the class if I cannot catch the different exception classes thrown with multiple catch statements, but would have to look at the messages.

And because you are asking about making that code usable to others: Use namespaces, PSR-0 compatible autoloading and your own exception classes. And do not grab data from superglobals.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top