0
\$\begingroup\$

I enter in the IMDb link and YouTube trailer link in the command line to a movie and the first main program loads all the info about the movie. The second main program uses an IMDb link to the movie and loads the actor's name and birthday.

Would it be better to write these programs as a class of some sort and use methods instead of just doing include statements?

I'm going to start writing a program to take these returned arrays and load them into a database. Please let me know what you all think and any improvements I can do! This is my first PHP program so I have a feeling these methods are a little unorthodox. I would like to know of alternative ways of doing it.

<?php include 'includeFunctions.php';?>
$data=curl($argv[1]);
$data=htmlspecialchars($data);
$movieInfo=[
    "title" => scrape_between($data, "&lt;title&gt;", "- IMDb"),
    "duration" => scrape_between($data, "&lt;time itemprop=&quot;duration&quot; datetime=&quot;PT", "&quot;&gt;"),
    "releaseDate" => scrape_between($data, "&lt;meta itemprop=&quot;datePublished&quot; content=&quot;", "&quot; /&gt;"),
    "description" => scrape_between($data, "&lt;div class=&quot;summary_text&quot; itemprop=&quot;description&quot;&gt;", "&lt;/div&gt;"),
    "imdbRating" => scrape_between($data, "itemprop=&quot;ratingValue&quot;&gt;", "&lt;/span&gt;&lt;"),
    "RTRating" => "",
    "youtubeTrailer" => $argv[2]
];
$movieInfo["description"] = trim($movieInfo["description"]);
$movieInfo["poster"] = scrape_between($data, "Poster&quot;", "itemprop");
$movieInfo["poster"] = scrape_between($movieInfo["poster"], "src=&quot;", "&quot;");
$titleString=$movieInfo["title"];
$titleString=explode(" ", $titleString);
$RTSearchURL="https://www.rottentomatoes.com/search/?search=" . $titleString[0];
for($i=1; $i<count($titleString)-1; $i++){
    $RTSearchURL.="%20" . $titleString[$i];
}
$data=curl($RTSearchURL);
$dataArray=explode('SummaryResults', $data);
for($i=0; $i<count($dataArray); $i++){
    $dataArray[$i]=htmlspecialchars($dataArray[$i]);
}
$newUrl='www.rottentomatoes.com' . scrape_between($dataArray[1], "href=&quot;", "&quot;&gt;");
$data=curl($newUrl);
$data=htmlspecialchars($data);
$RTRating=scrape_between($data, "Average Rating: &lt;/span&gt;", "/10");
$movieInfo["RTRating"]=trim($RTRating);
return $movieInfo;
}
?>

Program to get actor info:

<?php include 'includeFunctions.php';?>

<?php
$data=curl($argv[1]);
    $data=htmlspecialchars($data);
$actorArray=explode("class=&quot;itemprop&quot; itemprop=&quot;actor&quot;", $data);
$nameArray2d=array();
$count=0;
    for($i=0; $i<count($actorArray); $i++){
    $name=scrape_between($actorArray[$i], "temprop=&quot;name&quot;&gt;", "&lt;/span&gt;");
    $link=scrape_between($actorArray[$i], "&lt;a href=&quot;/name/", "&quot;");
    $link="www.imdb.com/name/" . $link;
    $actorData=curl($link);
    $actorData=htmlspecialchars($actorData);
    $actorDOB=scrape_between($actorData, "&lt;time datetime=&quot;", "&quot;");
    if($actorDOB!==""){
        $nameArray=explode(" ", $name);
        if(count($nameArray)==2){
            $nameArray2d[$count]=array($nameArray[0], $nameArray[1], $actorDOB);
        }
        else{
            $nameArray2d[$count]=array($nameArray[0], $nameArray[1], $nameArray[2], $actorDOB);
        }
        $count++;
    }
    else{
    }
    }
var_dump($nameArray2d);
?>

curl function to retrieve HTML code:

<?php
 function curl($url){
$options=Array(
//return data as a string instead of outputting
//it directly
CURLOPT_RETURNTRANSFER => TRUE,
//Follow any Location: headers that the server
//sends as part of the HTTP header ??
CURLOPT_FOLLOWLOCATION => TRUE,
//Automatically set the Referer: field in 
//request where it follows a Location: 
//redirect
CURLOPT_AUTOREFERER => TRUE,
//Number of seconds to wait while trying to
//connect
CURLOPT_CONNECTTIMEOUT => 120,
//Max numver of seconds to allow curl functions
//to execute
CURLOPT_TIMEOUT => 120,
//max number of redirects it will follow (used in
//conjunction with CURLOPT_FOLLOWLOCATION
CURLOPT_MAXREDIRS => 10,
);
$ch=curl_init($url);
curl_setopt_array($ch, $options);
$data=curl_exec($ch);
curl_close($ch);
return $data;
}
?>

scrape_between method:

<?php
/*Function to return specific data chunks in an array for
further scraping*/
function scrape_between($data,$start,$end){
//strips data string of everything up to AND
//including $start
$data=stristr($data, $start);
//returns the starting position of where $end
//starts
$data=substr($data,strlen($start));
$endPos=stripos($data, $end);
//creates a substring of just the data that we
//want
$data=substr($data, 0, $endPos);
//return the data
return $data;
 }
?>

include statements:

<?php
include 'scrape_between.php';
include 'curl.php';
?>
\$\endgroup\$
  • 2
    \$\begingroup\$ Why scrape? Is there not API which would be useful and less fragile? If you are going to download a lag, why not use DOM parser rather than build your own string manipulation process? \$\endgroup\$ – Mike Brant Sep 6 '16 at 3:41
1
\$\begingroup\$

Don't use count in a loop

for($i=0; $i<count($dataArray); $i++){
    $dataArray[$i]=htmlspecialchars($dataArray[$i]);
}

The count function in PHP is not particularly efficient. There are a couple other patterns that can be used. In this case, I would probably use

for ($i = 0, $n = count($dataArray); $i < $n; $i++) {
    $dataArray[$i] = htmlspecialchars($dataArray[$i]);
}

This only calls count once per loop, rather than once per iteration.

Another possibility is

for ($data as &$datum) {
    $datum = htmlspecialchars($datum);
}
unset($datum);

This is more complicated though. See the PHP manual for more information on why you have to use unset each time you do a foreach by reference.

Read only

for($i=0; $i<count($actorArray); $i++){

In this case, I'd use the foreach form.

for ($actorArray as $actor) {

Since you don't assign to $actorArray[$i], you don't have to unset the reference variable afterward.

Simplify

//strips data string of everything up to AND
//including $start
$data=stristr($data, $start);
//returns the starting position of where $end
//starts
$data=substr($data,strlen($start));
$endPos=stripos($data, $end);
//creates a substring of just the data that we
//want
$data=substr($data, 0, $endPos);
//return the data
return $data;

This could be shorter.

$startPosition = stripos($data, $start) + strlen($start);
$endPosition = stripos($data, $end, $startPosition);

return substr($data, $startPosition, $endPosition);

Comments should describe things that are not obvious in the code. They should not just tell us what the functions are doing. It's our responsibility to know what functions do.

Rather than create three substrings (one with stristr and two with substr), this only creates one substring.

This replaces stristr with stripos. Instead of using stristr and substr calls to strip off everything include the start string, I waited to the end to do that. To keep from processing all that a second time, I used an offset with the second stripos call.

Consistent use of "" and ''

$newUrl='www.rottentomatoes.com' . scrape_between($dataArray[1], "href=&quot;", "&quot;&gt;");

You use double quotes sometimes and single quotes other times. It's not clear to me why. The difference between them is that double quotes allow for variable interpolation.

My personal rule of thumb is to always use single quotes unless I want the benefits of variable interpolation.

\$\endgroup\$
  • \$\begingroup\$ scrape_between() function should use DOMDocument for easier string interrogation and management. \$\endgroup\$ – ʰᵈˑ Sep 6 '16 at 13:08
  • \$\begingroup\$ @ʰᵈˑ Why not write up an answer saying that? Note that as the application is currently written, scrape_between can't use DOMDocument, because the content was escaped prior to scrape_between being called. \$\endgroup\$ – mdfst13 Sep 6 '16 at 14:42
  • \$\begingroup\$ because you've pretty much covered everything except that one point. \$\endgroup\$ – ʰᵈˑ Sep 6 '16 at 15:13
1
\$\begingroup\$

I question the overall approach of using scraping for this. While it appears imDB does not support an official API, it does appear there are a number of community-driven API's around imDB that may offer you better functionality via a simple integration. I would consider taking a look at these.

If you want to stick with retrieving and manipulating the HTML document itself (whether this be for learning purposes or whatever), I would consider looking at DOMDocument and related classes to allow for more flexible DOM manipulation and information retrieval.

I think @mdfst13 pointed out a number is areas for potential improvement, so I will add a few more thoughts in addition to those provided.

  • Many lines of code are too long. Keeping lines to < ~80 characters is recommended across most style guides you see. Break your code up across lines to make it easier to read.
  • You have no comments or blank lines to break up the code into logical sections. Using some blank vertical space makes code much easier to read.
  • You should add spaces between operators and variables names.
  • Indent your code properly. There are a few files there without any indentation at all.
  • With these notes above, the overall takeaway should be that you have really densely packed code that is hard to read. You do not get bonus points in most any modern programming language for writing your code in as few lines/characters as possible, so why not make your code easier to read and work with?
  • You should consider decoupling your configuration (i.e. the movie "fields" you are looking to populate and the related search strings) from the code itself. You want to make the configurations as easily readable and understandable as possible, not first appearing as a parameter in a function call buried in your code.

Example:

$movieSearches = [
    'title' => [
        'start' => '&lt;title&gt;',
        'end' => '- IMDb'
     ],
     'duration' => [
         'start' =>
             '&lt;time itemprop=&quot;duration&quot; datetime=&quot;PT',
         'end' => '&quot;&gt;'
     ],
     // release date, description, imdb rating, poster, etc.
];
  • Now, having split out configuration, it becomes much easier to map the screen scraping results into an array with a few lines of code.

Example:

$movieInfo = [];
foreach ($movieSearches as $key => $searchConfig) {
    $movieInfo[$key] = scrape_between(
        $data,
        $searchConfig['start'],
        $searchConfig['end']
    );
}
$movieInfo['RTRating'] = '';
$movieInfo['youtubeTrailer'] => $argv[2];
// etc.
  • Don't drop in and out of PHP unnecessarily like in the example below. Doing this just adds spurious output.

Example:

 <?php include 'includeFunctions.php';?>

 <?php
  • Consider building classes to describe what movies and actors are in your system. Consider having other classes to contain your scraping (or perhaps eventually API or DOM manipulation) logic to build the movie and actor classes.
  • Try to eliminate usage of multiple if-else nests. Oftentimes you might find yourself needing to faili out of a process or needing to continue to next iteration. Try to evaluate conditions where you need to alter "happy path" program early within a function, method, code block, etc. as early in the code as practical, leaving the more complex "happy path" code until after these program flow decisions are made. This really helps in debugging code quickly, because the edge cases you account for are all pushed up to earlier spots in the code.

Here is an example of how a block of your code might be rewritten:

if($actorDOB!==""){
    $nameArray=explode(" ", $name);
    if(count($nameArray)==2){
        $nameArray2d[$count]=array($nameArray[0], $nameArray[1], $actorDOB);
    }
    else{
        $nameArray2d[$count]=array($nameArray[0], $nameArray[1], $nameArray[2], $actorDOB);
    }
    $count++;
}
else{
}

Could become:

// this replaces your outer if-else
if($actorDOB === '') {
    continue;
}

// happy path continues
// your pre-existing if-else logic is not needed here
$nameArray2d[$count] = explode(" ", $name);
$nameArray2d[$count][] = $actorDOB;
$count++;
  • Note that in my code example above, I also just simplified away your if-else logic around building this array. When you see sides of an if-else branch that look to do almost the same exact thing, look for ways to simplify your logic and remove the conditional.
\$\endgroup\$
  • \$\begingroup\$ @user3736114 Just made an update to capture a few more thoughts. \$\endgroup\$ – Mike Brant Sep 6 '16 at 19:34

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.