2
\$\begingroup\$

I have this Facebook class and I'm wondering if the OOP nature of it could be improved at all?

(GitHub project here):

<?php

class Facebook {

    private $baseUrl = 'https://graph.facebook.com/';

    public function basicInfo($username){

        if($username !== ''){
    			$url = $this->baseUrl . $username;
    		}
    		if($url !== ''){
            $data = $this->__apiCall($url);
    		}
    		return $data;

    }

    public function userProfilePicture($username, $size = 'square'){

        if($username !== ''){
    			$url = $this->baseUrl . $username . '/picture?type=' . $size;
    		}
    		return $url;

    }

    public function searchPublicPosts($query){

        if($query !== ''){
    			$url = $this->baseUrl . 'search?q=' . urlencode($query) . '&type=post';
    		}
    		if($url !== ''){
            $data = $this->__apiCall($url);
    		}
    		return $data;

    }

    public function searchPages($page){

        if($page !== ''){
    			$url = $this->baseUrl . 'search?q=' . urlencode($page) . '&type=page';
        }

        if($url !== ''){
    			$data = $this->__apiCall($url);
    		}
    		return $data;

    }

    public function searchEvents($event){

        if($event !== ''){
    			$url = $this->baseUrl . 'search?q='.urlencode($event).'&type=event';
    		}
    		if($url !== ''){
            $data = $this->__apiCall($url);
    		}
    		return $data;

    }

    private function __apiCall($url){

        $raw = file_get_contents($url);
        $json = json_decode($raw, true);
        return $json;

    }

}
\$\endgroup\$
6
\$\begingroup\$

This answer is in two sections. Firstly I am going to cover the Object Oriented nature of your solution and suggest possible architectural changes. Then I am going to look at your existing Code line by line for improvements.

Object Oriented

Your OO is good, you have nothing bad in there.

Here are some of the features of OO that you have used:

  • Data hiding (baseUrl property is private)
  • Encapsulation (use of private __apiCall to minimize interface for callers)

Lets have a look to see what other features you could use.

Now, what is your class? It seems that it is a URL builder for the Facebook API. You could think about creating a class that helps you with this URL building. Let me pull one out of my hat now... (untested, only syntax checked)

class UrlBuilder {
   protected $baseUrl;
       protected $domain;
   protected $scheme;

   public function __construct($scheme, $domain, $baseUrl) {    	   
    	   $this->baseUrl = $baseUrl;
    	   $this->domain  = $domain;
    	   $this->scheme  = $scheme;
   }

   /** Build a URL.
    *  @param rel   @string The relative offset from the baseUrl.
    *  @param query @array Associative array of query parameters.
    *  @return @string URL string.
    */
   public function buildUrl($rel, Array $query) {
       $url = $this->scheme . "://" . $this->domain . '/' . $this->baseUrl .
           $rel;

       if (!empty($query)) {
    		   $url .= '?' . http_build_query($query);
       }

       return $url;
   }
}

You will see from above that I use http_build_query (which is covered in the Code section of the review).

I left parameter checking out of this class to make this answer easier to read. However you should definitely check each scalar parameter $scheme, $domain, $baseUrl, $rel. Here is an example of how you would check $rel:

if (!is_string($rel)) {
    throw new InvalidArgumentException(__METHOD__ . 'rel must be a string');
}

So, how do we go about using this? The common initial thought is the fantastic OO feature inheritance! Raise your hand if you want to use inheritance. Luckily from here I can't see any hands raised. What would happen if we used inheritance (class Facebook extends UrlBuilder)? We would open up the buildUrl method allowing any URL to be built with your Facebook object. This would be bad. Why was inheritance a bad idea? Because the Facebook object is not a UrlBuilder. It is something that uses URL building.

The answer is composition. We should pass the URL builder that we want to use into our constructor (generally I would use an interface, lets call it "UrlBuilderIface" and have class UrlBuilder implements UrlBuilderIface). Using an interface allows us to keep the class loosely coupled. We can accept any object that fulfills the contract defined by our UrlBuilderIface. Here is the implementation with one method defined:

class Facebook {
    protected $urlBuilder;

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

    public function searchPublicPosts($query) {
            if (empty($query) || !is_string($query)) {
            throw new InvalidArgumentException('query must be a non empty string.');
        }

        return $this->__apiCall($this->urlBuilder('search', array('q' => $query)));
    }

    private function __apiCall($url) {
            $raw = file_get_contents($url);
            return json_decode($raw, true);
    }
}

Note how there is checking for the passed in parameter to the method. The check is simple, and it makes the rest of the code easy. It is also easy to think about it completely separately from the other code (because you know that an exception will be thrown if the guard condition is met).

Usage becomes:

$facebookGraphUrlBuilder = new UrlBuilder('https', 'graph.facebook.com', '');
    $facebook = new Facebook($facebookGraphUrlBuilder);

Code

Some minor comments:

  • __apiCall should be named apiCall. __ should be reserved for magic methods.
  • json_decode does not produce json, so you shouldn't create a variable from it called $json.
  • $json should never have even been a variable (A variable is for varying). So you should return straight away, return json_decode($raw, true);

I am going to look at a single method because you have the same problems shared across your methods.

public function basicInfo($username){

        if($username !== ''){
                $url = $this->baseUrl . $username;
            }
            if($url !== ''){
            $data = $this->__apiCall($url);
            }
            return $data;

    }

Observe what would happen if $username was '':

  • First if does not match, so $url is not set.
  • Non-existent $url is used in second if causes E_NOTICE error.
  • Second if matches as NULL !== '' (This is not good).
  • apiCall is made using NULL.
  • Badness continues.

Actually, similar or worse would happen if you used NULL an array, etc.

The bad thing here is the combination of your conditional setting of variables and later expectation that the variables have been set. I would recommend solving this using a guard condition to ensure that the parameters to your methods are sane. Trying to continue with the processing using if statements is quite difficult. Throwing an exception may be the easiest:

if (!empty($username) || !is_string($username)) {
    throw new InvalidArgumentException('username required as a non-empty string.');
}

Everything can then be assumed to work:

return $this->__apiCall($this->baseUrl . $username);

Finally, there is a PHP function that helps you build the query strings that you use. Here is the manual page for it: http_build_query

\$\endgroup\$
  • \$\begingroup\$ Wow. Well this is fantastic. I'm definitely going to be using this site more often, if this is the calibre of review. This will help my coding no end! \$\endgroup\$ – benhowdle89 Apr 23 '12 at 18:25
  • 2
    \$\begingroup\$ I'm glad it was helpful. I love OO, so I put some extra effort into this answer. \$\endgroup\$ – Paul Apr 24 '12 at 2:25

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.