Understanding Object-Oriented Programming - What can I improve?

Question

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:

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?
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?
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!

If it's using cURL, it should most definitely ensure that the stuff it needs exists. At the very least, the file that defines the class should check. It shouldn't exit, though; it should probably be throwing an exception instead. — cHao
– cHao, Commented Jan 16, 2014 at 0:28
BTW, considering this is a request for improvements to working code, it'd probably be more appropriate for Code Review. Questions here are typically expected to involve stuff that doesn't work. — cHao
– cHao, Commented Jan 16, 2014 at 0:30
Also, PHP already has a built-in way to do the $method and $parameters thing. Were your callData function renamed to __call, the caller would never even have to know the method is dynamic; it can just say $obj->someMethod($params). This makes it pretty easy to build a proxy object representing the underlying API, without making things ugly for the caller. — cHao
– cHao, Commented Jan 16, 2014 at 0:42

Tobias · Accepted Answer · 2014-01-16 00:53:20Z

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.

Ah thank you, that makes way more sense! When you say "// add method support, so you don't need an extra POST method" - can you just expand on that one?

bishop · Accepted Answer · 2014-01-16 00:30:23Z

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.
Create an abstract class that implements your interface and extend off of that. Or use PHP 5.4 traits.
Maybe. I would rather see a method call corresponding to each method in your API. But it's one of preference.

answered Jan 16, 2014 at 0:30

bishop

39.8k14 gold badges116 silver badges150 bronze badges

Collectives™ on Stack Overflow

Understanding Object-Oriented Programming - What can I improve?

2 Answers 2

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related