Two similar API calls PHP optimisation and structure

Question

I have the following script that I'm part way through and I'm not happy with it at all, it feels messy:

<?php
class Process {
    public $search = 'http://api.themoviedb.org/2.1/Person.search/en/json/xxx/';
    public $info = 'http://api.themoviedb.org/2.1/Person.getInfo/en/json/xxx/';

    public function returnId($name){
        return $this->returnJson($this->search . $name);
    }

    public function returnMovies($id){
        return $this->returnJson($this->info . $id);
    }

    public function returnJson($url){
        $raw = file_get_contents($url);
        $json = json_decode($raw, true);
        return $json;
    }
}

$p = new Process();
$first = urlencode($_POST['first']);
$second = urlencode($_POST['second']);

$id = $p->returnId($first);
$movies = $p->returnMovies($id[0]['id']);

Now the idea is that two parameters get sent to this script and each of them run through the first API call ($search) and then the ID field is extracted from that results and passed into the $info URL call. The two arrays that come back from returnMovies will be compared with each other and any matches will be popped into another array.

Now, has anyone got any way for this to be better in structure and cleanliness?

It's not finished yet but its a simple script that I'd like to tidy up before I go on.

Answer 1

I'd start firstly by moving your urlencode() into the class... You should assume that everything passed to a method is unclean and therefore act appropriately.

Secondly, I'd give the class a meaningful name - Process isn't entirely descriptive.

Throw some comments in - follow a set standard such as the PHPDoc format, using @param, @return etc. Always handy to have, even if the code doesn't become public or used by others.

Look at your public/private declarations - or lack of. Personally, I'd look to make your $search and $info URLs private, as well as your returnJson() method.

I'd possibly look to use cURL instead of file_get_contents() - due to some server configs disabling that etc.

Anyway, just a few suggestions!

Answer 2

I agree with Michael Wright completely +1, I only have a few additions.

If both of those URLs are that similar you should use a URL header and append the differences to each in their respective variables. Saves having to change it multiple times should the URL ever change.

private $urlHeader = 'http://api.themoviedb.org/2.1/';
public $search = $url_header . 'Person.search/en/json/xxx/';
public $info = $url_header . 'Person.getInfo/en/json/xxx/';

I would try and find some way of combining returnId() and returnMovies(). As they are so similar it would be a shame not to reuse it. I first thought to use a switch statement, but that would create as much work and mess as using two separate methods, so thats no good. Second, I thought of variable-variables. I don't much like variable-variables, but it was the first "clean" idea to pop into my head. I say that last with a shudder. However, a third solution came to me as I was writting the variable-variable solution out. If you don't use those URL variables ($search and $info) anywhere else, you could just set it on the fly.

public function returnInfo($search, $method = 'search'){
    $url = "http://api.themoviedb.org/2.1/Person.$method/en/json/xxx/";
    return $this->returnJson($url . $search);
}

Because each method basically only changed a part of the URL before passing it to returnJson(), I passed that part as an argument to save time. I also gave it a default setting so it wouldn't always have to be passed. However, for legibility, this is only advisable if you find yourself using more than the two methods you already have.

Here's the variable-variable solution, also with a default URL.

public function returnInfo($search, $url = 'search'){
    return $this->returnJson($this->$url . $search);
}

Disclaimer: I am not condoning the use of variable-variables, merely showing it. I wouldn't have even shown it if I hadn't already typed it, but I did, so here it is. I really would not suggest its use. They are very confusing, even if well documented.

And finally, since you are only using returnJson() inside the class, you should make it a private method. No need to give everyone access to it if you don't plan on letting them use it.