This project is read-only.

Code review: Exception handling - try, throw, catch, echo, die

Topics: Help Section
Aug 11, 2009 at 8:02 PM

I don't understand the reasons for doing it this way.

Here is an example

 

    /**
     * The search term which you will query the server
     * @param string $query
     * @return object
     */
    public function query($query) {
        try {

            $this->_queryTerms = $query;
            if(empty($this->_queryTerms)) { throw new Exception('We can\'t search without an input'); }
            
        } catch(Exception $e) {
            echo "An error has been detected: ".$e->getMessage();
            die();
        }

        return $this;
    }

 

 

  1. You throw an catch the *same* exception in the *same* method, that doesn't make sense. The caller is the one to handle exceptions, your only responsibility is to ensure that the state of your object is sane, and let the developer know if your object can be reused even in the case of an exception.
  2. If you want to "echo" the error message, you can use PHP's trigger_error
  3. Don't die! Pretend I'm half-way through generating the HTML content for my page, the start of the HTML is already sent to the client, and then - bom - blank. No content at all! If this webapp is using several 3rd party components, your error message doesn't clearly say "Hello, It's the BingAPI that caused this".
  4. If you choose to only use exceptions (and not trigger_error), create your own BingException and clearly let the developer know this is how you want him/her to use the class. Check out W3CSchools example for creating a custom exception.

You provide public fields, a field prefix with underscore ( public $_version = "2.0" ) usually implies that the field is suppose to be private, and that your class knows best how to set/get the values. I don't see any magic methods in case you don't feel the urge to make getters/setters for these fields.

Also you use CURL, as far as I know, and I just test-installed PHP 5.3.0 today, I had to enable the ext_curl extension. You would want to use function_exists to determine if CURL is enabled, if you don't provide a replacement for CURL, this is a good point to use trigger_error with E_USER_ERROR. See constants for trigger_error.

In setProxy, your intention is to let the developer know that he cannot use this method. You can use trigger_error with E_USER_DEPRECATED, I assume, it's intended usage.

I am not sure if php's switch cares about Xml vs xml and XML. in setFormat, if it can't tell the difference between Xml and xml, you should use switch(strtolower($format)).

Also, if your method throws an exception, it is *extremely* important to document this feature. Not only the fact that it throws an exception, but also what type of exception (ie: BingException)

Btw, the proper place to check if CURL is loaded, is probably the constructor, as early as possible.

Documentation helpers:

  • I don't know what a SourceType ($_sources) is, or why you choose Web or Image, where do those strings come from?
  • What's $_market for, what other values might I use?
  • $_options say: An array containing optional parameters - parameters to where, any example options? What kind of array - $opts[] = "opt1" or $opts["opt1"] = "value"?

Anyway, this post got abit longer than I expected, but I just wanted to give some feedback. I initially wanted to port this code to C# (.NET), but ended up writing this instead.

Best regards!

Aug 11, 2009 at 10:44 PM

Hello there! CoolSpin :)

I really thank you for your feedback. I was starting to lose hope that no one was pointing out my errors and typos for me to amend (No, not being sacarstic)

 I've a version *not public* that fixes some of your points, but ah well I'll get to that.

1. I did that because I always had "uncaught exception" with a backtrace, and while I was on my testing I thought adding the try/catch/exceptions would help which was also influenced when I was doing the samples.
2. Yes, I'm aware of trigger_error, but it looks like I didn't think of them back then, will implement it this time.
3. I added the die() which was also influenced by doing the sample code, I guess I'll take it to revision.
4. I already thought of creating my custom exception, it just never happened but will do.

setFormat will have strtolower in the next release, I already added it.

As for documentation helpers, noted :) though $_market is going to be removed since it just goes along with the options array when querying the bing server

 Once again thank you for your time and good luck porting the code :)

Best wishes,
David

Aug 18, 2009 at 2:26 PM

Hi

I think its great you have progressed to recognize previous mistakes. And that you appreciated my feedback.

I urge you to try TortoiseSVN and use it actively when developing projects. TortoiseSVN can be used on CodePlex. You can have the files that are currently in development in trunk, this also provides a way for others to give feedback.

Source Code Control can be used for application- and web-developers, authors (ie. documentation) and for configs. And anything else you might need to keep track of. There is lots of resources about the subject.

I'm going to assume there is a pending release, and wait for that before I begin doing any work.

Best regards!

Aug 21, 2009 at 12:56 AM
CoolSpin wrote:

Hi

I think its great you have progressed to recognize previous mistakes. And that you appreciated my feedback.

I urge you to try TortoiseSVN and use it actively when developing projects. TortoiseSVN can be used on CodePlex. You can have the files that are currently in development in trunk, this also provides a way for others to give feedback.

Source Code Control can be used for application- and web-developers, authors (ie. documentation) and for configs. And anything else you might need to keep track of. There is lots of resources about the subject.

I'm going to assume there is a pending release, and wait for that before I begin doing any work.

Best regards!

 I'm aware of SVN and have used it before but for something small as this tiny library I doubt I need to do something like that. I know where you are going with this although I feel its just going to become a hassle for me.

There is a pending release, yes. It's going to take a while too since I have my hands full with college stuff and another heavy project (at least I believe it's heavy) that I'm developing to release. Many stuff going on in the developing of that certain project, since it's my first time implementing the MVC and lacking a lot of content regarding patterns (especially implemented in PHP) and object interaction (although I pretty much have it solved).

To those reading, the license will change to the MSPL (Microsoft Public License). Pretty much similar to the MIT License, although it just to please the corporate lawyers up in microsoft.