CODEWITHSUNDEEP
phpoop
Alias
Alias

Reputation: 3081

Understanding Object-Oriented Programming - What can I improve?

Cutting to the chase: There is an API which my application will be using to make calls to, using cURL. However this API needs to be kept secret, so I'm using a framework to create my own API to query the external API. I have done this, but it's really messy and is bad coding - so I want to do this as properly as I can, for my own learning.

I first create my interface:

interface APICall {

    /**
     * Return data from the API
     * @returns json
     */
     public function callData($method, $parameters);

}

I then create my class, which will be doing the cURL (will just do a GET request for now):

class curl {

    private static $apiUrl = 'http://api.somewebsite.com/v1/';

    public function __construct() {

        if (!function_exists('curl_init')) 
            exit('CURL is not installed!');

    }

    public function getCurl($method, $parameters) {

        $url = self::$apiUrl . $method . '?' . $parameters;

        $ch = curl_init();
        curl_setopt($ch, CURLOPT_URL, $url);
        curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
        $output = curl_exec($ch);
        curl_close($ch);

        return $output;

    }

    // Another function for a POST request could go here

}

Okay, so now I'd create my classes for each specific call, for example "Get the list of users":

class users extends curl implements APICall {

    /**
     * Get list of users
     */

    public function callData($method, $parameters) {

        $this->getCurl($method, $parameters);

    }

}

Okay - now I'm not entirely sure this will work (brainstorming for now), but I do feel I'll have some issues:

  1. The __construct in my curl class, I don't think this should be containing a "check if cURL is installed" check - but where would this best go?

  2. I'm building the $url within each method in the curl class - this just seems bad since I'll end up repeating this - but where do I create that to be used?

  3. I feel as if I'm using $method & $parameters quite frequently, is this normal?

Sorry if this is quite a bit, just trying to understand it as my current coding practices are crap!

Upvotes: 1

Views: 101

Answers (2)

Tobias
Tobias

Reputation: 954

class curl {
    private $method;
    private $parameters;
    private $url;

    public function __construct($url,$method,$parameters = array()) {
        if (!function_exists('curl_init')){ 
            throw new Exception('CURL is not installed!');
        }
        $this -> setUrl($url);
        $this -> setMethod($method);
        $this -> setParameters($parameters);        
    }

    public function setMethod($method){
        $this -> method = $method;
        return $this;
    }

    public function getMethod(){
        return $this -> method;
    }

    public function setParameters(array $parameters){
        $this -> parameters = $parameters;
        return $this;
    }

    public function getParameters(){
        return $this -> parameters;
    }

    public function setUrl($url){
        $this -> url = $url;
        return $this;
    }

    public function getUrl(){
        return $this -> url;
    }

    public function execute() {    
        // add method support, so you don't need an extra POST method   
        $url = $this -> createUrl();
        $ch = curl_init();
        curl_setopt($ch, CURLOPT_URL, $this -> getUrl());
        curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
        $output = curl_exec($ch);
        curl_close($ch);

        return $output;
    }

    private function createUrl(){
        return $this -> url . $this -> getMethod() . '?' . http_build_query($this -> getParameters());
    }
}

You don't need to use the constructor, but I think a curl without url and method is senseless. So you can create a dependency to url and method by putting it into the constructor. Add Setter and Getter Support to change the state of the object from outside. You don't need to create different execute functions, one is enough. The problem with the url creation is also solved. Set the url from outside. So you can use the curl in other contexts, too.

Try to inject the curl class and don't extend it. Example:

class specificApi implements ApiInterface {
    private $curl;

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

    public function execute(){
        return $this -> curl -> setUrl('someUrl') -> setMethod('someMethod') -> execute();
    }
}

In the next step you'd recognize that curl always needs some constructor parameters, so you'd create a factory that creates curl classes. You'd inject the factory and create an instance of curl class in the api class.

Upvotes: 1

bishop
bishop

Reputation: 39364

  1. Either in the code installer (eg, emerge, webapp-config, rpm, etc), a dedicated /setup route in your framework, or at the top of the file when required. Only in the constructor in the worst case.

  2. Create an abstract class that implements your interface and extend off of that. Or use PHP 5.4 traits.

  3. Maybe. I would rather see a method call corresponding to each method in your API. But it's one of preference.

Upvotes: 1

Related Questions