2
\$\begingroup\$

I've put together a class that grabs data from imdb.com or allmusic.com, then inserts the data into a database, copy's the poster, also updates the data if updateData() is called. I'm pretty new to OOP so I'm wondering what I'm doing right/wrong? any advice really.

class mediaMetaData {

    public $imdb;
    public $allmusic;
    public $data = array();
    protected $url;
    protected $imagePath;
    protected $db;
    protected $dbTable;


    public function __construct( $tag, $db ) {

        $this->setTag( $tag );
        $this->db = $db;
        switch (true) {
            case $this->is_imdb():
                $this->dbTable = 'imdb';
                $this->url = 'http://www.imdb.com/title/' . $this->imdb . '/';
                $this->imagePath = IMAGES_ROOT . '/movieposters/' . $this->imdb . '.jpg';
            break;
            case $this->is_allmusic():
                $this->dbTable = 'allmusic';
                $this->url = 'http://www.allmusic.com/album/' . $this->allmusic . '/';
                $this->imagePath = IMAGES_ROOT . '/albumartwork/' . $this->allmusic . '.jpg';
            break;
        }
        $this->data = $this->sortData();
    }


    protected function setTag( $tag ) {

        //convert the full url into a useable tag then make sure $tag is a string so it can be inserted into the db
        $this->imdb = $this->getMatch( '/.*(tt[0-9]{7}).*$/', $tag );
        $this->allmusic = $this->getMatch( '/.*(mw[0-9]{10}).*$/', $tag );

        switch (true) {
            case $this->is_imdb():
                $this->allmusic = '';
                return $this->imdb;
            break;
            case $this->is_allmusic():
                $this->imdb = '';
                return $this->allmusic;
            break;
            default:
                $this->imdb = '';
                $this->allmusic = '';
            break;
        }
    }


    //methods for checking and comparing. start
    public function is_imdb() {

        if( preg_match( '/^tt[0-9]{7}$/', $this->imdb ) ) {
            return true;
        } else {
            return false;
        }
    }


    public function is_allmusic() {

        if( preg_match( '/^mw[0-9]{10}$/', $this->allmusic ) ) {
            return true;
        } else {
            return false;
        }
    }


    public function contentExists( $string ) {

        if( strlen( $string ) > 0 ) {
            return true;
        } else {
            return false;
        }
    }


    protected function pageExists() {

        if ( empty( $this->data["error"] ) ) {
            return true;
        } else {
            return false;
        }
    }


    protected function tagExists() {

        $query = $this->selectData( 'tag' );
        if( $query ) {

            if( is_array( $query ) ) {
                return true;
            } else {
                return false;
            }
        }
    }
    //methods for checking and comparing. end


    //get and sort the metadata. start
    protected function getData() {

        $ch = curl_init();
        $timeout = 5;
        curl_setopt( $ch, CURLOPT_URL, $this->url );
        curl_setopt( $ch, CURLOPT_RETURNTRANSFER, 1 );
        curl_setopt( $ch, CURLOPT_CONNECTTIMEOUT, $timeout );
        $data = curl_exec( $ch );
        curl_close( $ch );
        return $data;
    }


    protected function getMatch( $regex, $content ) {

        preg_match( $regex, $content, $matches );
        return $matches[1];
    }


    protected function copyImage( $image ) {

        if( file_exists( $this->imagePath ) ) {
            return false;
        } else {
            return copy( $image, $this->imagePath );
        }
    }


    protected function sortData() {

        //get the content from imdb and put it into a array
        $content = $this->getData();

        switch (true) {
            case $this->is_imdb(): 

                $regex = [
                        'error' =>  "/<title>(.*) Error - IMDb<\/title>/isU",
                        'title' => "/<div class=\"title_wrapper\">.*class=\"\">(.*)&nbsp;<span/isU",
                        'poster' => "/<div class=\"poster.*src=\"(.*)\"/isU",
                        'director' => "/<h4[^>]*>Director:<\/h4>(.*)<\/span>/isU",
                        'plot' => "/<div class=\"plot_summary.*description\">(.*)<\/div>/isU",
                        'rating' => "/<span itemprop=\"ratingValue\">(.*)<\/span>/isU",
                        'metascore' => "/<div class=\"metacriticScore.*<span>(.*)<\/span>/isU",
                        'release_date' => "/Release Date:<\/h4>(.*)<span/isU",
                        'cast' => "/<div class=\"article\" id=\"titleCast\">(.*)See full cast/isU"
                ];
            break;
            case $this->is_allmusic():

                $regex = [
                        'error' => "/<h1>(.*) Error: This page did not load properly<\/h1>/",
                        'cover' => "/div class=\"album-contain\">.*<img src=\"(.*)\"/isU",
                        'artist' => "/<h2 class=\"album-artist\".*<span itemprop=\"name\">(.*)<\/h2>/isU",
                        'album' => "/<h1 class=\"album-title\" itemprop=\"name\">(.*)<\/h1>/isU",
                        'overview' => "/div class=\"text\" itemprop=\"reviewBody\".*>(.*)<\/div>/isU",
                        'tracklist' => "/<section class=\"track-listing\">.*<div class=\"title\" itemprop=\"name\">(.*)<\/section>/isU",
                        'rating' => "/<span>AllMusic Rating<\/span>(.*)<\/div>/isU"
                ];
            break;
        }
        $output = array();
        foreach ( $regex as $key => $value ) {
            $$key = $this->getMatch( $value, $content );
			$output[$key] = $$key;
        }
        return $output;
    }


    public function insertData() {

        if ( !$this->tagExists() && ( $this->is_imdb() || $this->is_allmusic() ) ) {

            if( $this->pageExists() ) {

                // insert the data if the tag is new & the page exists.
                switch (true) {
                    case $this->is_imdb():
                        $insertArray = [
                                        'tag' => $this->imdb,
                                        'title' => $this->cleanString( $this->data["title"] ),
                                        'plot' => $this->cleanString( $this->data["plot"] ),
                                        'director' => $this->cleanString( $this->data["director"] ),
                                        'rating' => $this->cleanString( $this->data["rating"] ),
                                        'metascore' => $this->cleanString( $this->data["metascore"] ),
                                        'release_date' => $this->cleanString( $this->data["release_date"] ),
                                        'cast' => $this->formatCast(),
                                        'timestamp' => time()
                        ];
                        $image = $this->data["poster"];
                    break;
                    case $this->is_allmusic():
                        $insertArray = [
                                        'tag' => $this->allmusic,
                                        'artist' => $this->cleanString( $this->data["artist"] ),
                                        'album' => $this->cleanString( $this->data["album"] ),
                                        'overview' => $this->cleanString( $this->data["overview"] ),
                                        'tracklist' => $this->formatTracklist(),
                                        'rating' => $this->cleanString( $this->data["rating"] ),
                                        'timestamp' => time()
                        ];
                        $image = $this->data["cover"];
                    break;
                }
                $this->copyImage( $image );
                $this->db->insert( $this->dbTable, $insertArray );
            } else {
                $this->imdb = '';
                $this->allmusic = '';
                return false;
            }
        } else {
            return false;
        }
    }


    public function updateData( $id ) {

        //update the poster,rating,metascore and timestamp
        switch (true) {
            case $this->is_imdb():
                $updateArray = [
                                'rating' => $this->cleanString( $this->data["rating"] ),
                                'metascore' => $this->cleanString( $this->data["metascore"] ),
                                'timestamp' => time()
                ];
                $image = $this->data["poster"];
            break;
            case $this->is_allmusic():
                $updateArray = [
                                'rating' => $this->cleanString( $this->data["rating"] ),
                                'timestamp' => time()
                ];
                $image = $this->data["cover"];
            break;
        }
        $this->copyImage( $image );
        $this->db->update( $this->dbTable, $id, $updateArray );
    }


    public function selectData( $column = '*' ) {

        switch (true) {
            case $this->is_imdb():
                $data = $this->db->select( $this->dbTable, $column, ['tag' => $this->imdb] );
            break;
            case $this->is_allmusic():
                $data = $this->db->select( $this->dbTable, $column, ['tag' => $this->allmusic] );
            break;
            default:
                return false;
            break;
        }
        return $data;
    }
    //get and sort the metadata. end


    //format and output the data, start.
    protected function cleanString( $string ) {

        $cleanString = trim( preg_replace( '/\r|\n/', '', str_replace( '  ', '', strip_tags( trim( $string ) ) ) ) );
        return $cleanString;
    }


    protected function formatCast() {

        //make the cast nice and tidy.
        $data = explode( "<td class=\"primary_photo\">", $this->data["cast"] );
        $count = 1;
        $limit = 11;

        foreach ( array_slice( $data, 1, $limit ) as $value ) {

            $actor = $this->getMatch( "/<span class=\"itemprop\" itemprop=\"name\">(.*)<\/span>/isU", $value );
            $character = $this->getMatch( "/<td class=\"character\">(.*)<\/td>/isU", $value );

            $output .= $this->cleanString( $actor ) . " -- " . str_replace( ' /', '/', $this->cleanString( $character ) );
            $output .= ( $count < $limit ? "</br>" : "" );
            $count++;
        }
        return str_replace( "</br>", "\n", $output );
    }


    protected function formatTracklist() {

        $data = explode( "<div class=\"title\" itemprop=\"name\">", $this->data["tracklist"] );
        $numItems = count( $data );
        $count = 0;

        foreach ( $data as $value) {
            $track = $this->getMatch( "/itemprop=\"url\">(.*)<\/a>/isU", $value );
            $length = $this->getMatch( "/<\/span>(.*)<meta/isU", $value );

            $output .= $this->cleanString( $track ) . " -- " . $this->cleanString( $length );
            if ( ++$count !== $numItems ) {
                $output .= "</br>";
            }
        }
        return str_replace( "</br>", "\n", $output );
    }


    public function outputData() {

        switch (true) {
            case $this->is_imdb():
                $order = [
                        "title" => 0,
                        "plot" => 1,
                        "cast" => 0,
                        "rating" => 1,
                        "metascore" => 1,
                        "release_date" => 1,
                        "director" => 1
                ];
            break;
            case $this->is_allmusic():
                $order = [
                        "artist" => 1,
                        "album" => 1, "rating" =>1,
                        "overview" => 0,
                        "tracklist" => 0
                ];
            break;
        }
        $data = $this->selectData();

        if( $data ) {

            //update when viewed once a week.
            if ( time() > $data["timestamp"]+604800 ) {

                $this->updateData( $data["id"] );
            }

            //format & build the output string.
            ( $this->imageExists() ? $output .= "<img src=\"{$this->var["imagePath"]}\"></br>" : "" );

            foreach ( $order as $index => $title ) {
                if( $this->contentExists( $data[$index] ) ) {

                    if( $title === 1 ) {
                        $output .= "<b>" . ucwords( str_replace( "_", " ", $index ) ) . ":</b> ";
                    }
                    $output .= nl2br( $data[$index] ) . "</br>";
                }
            }
            return $output;
        }
    }
    //format and output the data, end
}

/*usage
//database
table 'imdb'
columns 'id' auto inc, 'tag', 'title', 'plot', 'cast', 'director', 'rating', 'metascore', 'release_date', 'timestamp'
table 'allmusic'
columns 'id' auto inc, 'tag', 'artist', 'album', 'overview', 'tracklist', 'rating', 'timestamp'

//insert into the database
$mediaMetaData = new mediaMetaData( "tt3450958", $db );
$mediaMetaData->insertData();
insert $mediaMetaData->imdb and/or $mediaMetaData->allmusic into a table

//display and update the data
$mediaMetaData = new mediaMetaData( "tt3450958", $db );
echo $mediaMetaData->outputData();*/

The database class I used:

class database {

    public $handle;


    public function __construct() {

        $this->handle = new PDO( "mysql:host=" . DB_SERVER . ";dbname=" . DB_DATABASE, DB_USER, DB_PASSWORD );
        $this->handle->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );
        return $this->handle;
    }


    public function selectAll( $table, $columns = "*", $order = false ) {

        $sql = "SELECT {$columns} FROM {$table}";
        ( $order ? $sql .= " ORDER BY " . $order : "" );

        return $this->handle->query( $sql )->fetchAll( PDO::FETCH_ASSOC );
    }


    public function select( $table, $columns = "*", $where = array() ) {

        $sql = "SELECT {$columns} FROM {$table}";

        if( !empty( $where ) ) {
            $sql .= " WHERE " . array_keys( $where )[0] . " = ?";
        }
        $query = $this->handle->prepare( $sql );

        $query->execute( ( !empty( $where ) ? array_values( $where ) : null ) );
        return ( empty( $where ) ? $query->fetchAll( PDO::FETCH_ASSOC ) : $query->fetch( PDO::FETCH_ASSOC ) );
    }


    public function insert( $table, $insertArray ) {

        $columns = array_keys( $insertArray );
        $placeHolders = array();

        foreach ( $insertArray as $value ) {
            $placeHolders[] = "?";
        }
        $query = $this->handle->prepare("
            INSERT INTO {$table}
            (" . implode( ',' ,$columns ) . ")
            VALUES
            (" . implode(",", array_values( $placeHolders ) ) . ")
        ");

        if( $query->execute( array_values( $insertArray ) ) ) {
            return true;
        }
    }


    public function update( $table, $id, $updateArray ) {

        $valuesArray = array_values( $updateArray );
        $valuesArray[] = $id;
        foreach ( $updateArray as $col => $value ) {
            $vals .= "$col = ?, ";
        }
        $vals = substr( $vals, 0, -2 );
        $query = $this->handle->prepare( "UPDATE $table SET $vals WHERE id = ?" );

        if( $query->execute( $valuesArray ) ) {
            return true;
        }
    }


    public function delete( $table, $id ) {

        $query = $this->handle->prepare( "DELETE FROM {$table} WHERE id = :id" );
        if( $query->execute( ['id' => $id] ) ) {
            return true;
        }
    }
}
\$\endgroup\$
1
\$\begingroup\$
  • Why use a switch(true) instead of just an if/else? The switch is 2 lines longer than an if would be and it's not easier to read. Perhaps you have a reason I'm not thinking of?
  • Why no fancy switch in the is_imdb() :P .. you can shorten that function to a single line.. no reason to put a boolean in an if else just so you can return a boolean.. just return preg_match( '/^tt[0-9]{7}$/', $this->imdb ) - Same goes for is_allmusic()
  • Same goes for the contentExists function.. just return strlen( $string ) > 0 - there is no reason to use an if/else there.
  • Ditto for the pageExists function.
  • Your copyImage function is backwards.. your only attempting to copy if a file does not exist. I think you forgot a ! in the condition.
  • Avoid using variable variables at the end of the sortData function, just assign the value directly to the array.
  • Bro.. these switch statements..
  • In your cleanString function you are creating a variable and then immediately returning it. That's memory that doesn't need to be used. Save yourself some memory and just return the value without assigning it to a variable first.
  • Don't parse HTML with REGEX
  • Good job !
\$\endgroup\$
  • \$\begingroup\$ I used the switch statements incase I wanted to add more but I see your point, I made this as a self taught beginner. On your other points I will apply them. I've learnt a few things here, thanks. \$\endgroup\$ – Benwhittaker25 Aug 22 '17 at 19:13
  • \$\begingroup\$ @Benwhittaker25 of course you did. that makes perfect sense :) \$\endgroup\$ – I wrestled a bear once. Aug 22 '17 at 19:16
1
\$\begingroup\$

At the end of your question you said "any advise really" so I'd like to offer some advise on why you should not place your logic and data into PHP objects with few exceptions.

PHP's classes were largely a response OOP's growing popularity in other languages, but it's implementation wasn't as well grounded as in other languages, the paradigm also in my opinion does not fit the problem space of PHP web applications very well.

When I say OOP I refer to Java/PHP like OOP, Alan Kay's original vision of OOP is more about message passing and late binding than it is Java objects see: https://medium.com/skyfishtech/retracing-original-object-oriented-programming-f8b689c4ce50

As a maintainer of PHP OOP code this is what I find problematic with PHP OOP code & of OOP coders:

  • All objects in PHP are copy by reference by default, this means changes you make in a function propagate backwards and out of the function by changing the original in place, but can even create problems in scope you'll often see this when dealing with PHP's DateTime object

  • Objects encourage invisible dependencies by storing things on $this in my eyes, $this is the new global, $this properties can be changed from anywhere at anytime in the life time of the object (inside and out), they also allow you to get things into methods without passing them in through the parameters making your methods hard to test and hard to type hint.

  • OOP coders have some mental ideas about why to set something private/protected

    • Some think it's to prevent other programmers miss using their object
    • Some think it makes properties immutable
    • Some set everything to private/protected because they were taught it but never questioned why
  • OOP coders contort their logic into objects and then find OOP usually doesn't provide a obvious structure

    • The problem is most developers will then assume the problem is they simply haven't found the abstraction yet:
      • Abstract Factory
      • Builder
      • Factory Method
      • Adapter
      • Bridge
      • Facade
      • Flyweight
      • Proxy
      • Mediator
      • Observer
      • Visitor
      • Null Object

You will see some horribly ugly contraptions as a result, when you're more focused on your abstraction you're less focused on creating good names and clear to read code, most OOP code is procedural code inside methods anyway so generally speaking you get worse procedural code with hidden $this variables jumping in everywhere

  • Objects encourage copy paste development, need a new object that acts similar to another one? copy and paste and try and refactor functions out into the parent object in, then all instances carry the full weight of the parent object, this is particularly bad in PHP because there's no multiple inheritance (except traits which is basically copy and paste)

That said: PHP only supports auto loading through objects

My advise is to use free functions (I was surprised C# don't have this luxury) namespaces and put your data into arrays you'll get copy by reference, you'll get interop with array_map array_reduce array_filter array_walk usort which become very important when trying to write code that describes what it does

If you need resources you won't find them written in PHP I suggest looking at JavaScript's functional renaissance, and don't go too academic into it because functional programming suffers the same (if not worse) problem of describing fairly simple concepts with exceptionally long words

Hope this gives another prospective without being too over bearing, this point of view isn't currently popular in the PHP word, we're still copying rails with laravel, rails itself is fairly old news now at 11 years old which is ancient in the web world, reactjs has taken some functional ideas that might be a better guide to better programming in PHP

\$\endgroup\$
  • \$\begingroup\$ Thanks for this, very insightful. I thought of a few things like the copy/paste stuff when building the class. \$\endgroup\$ – Benwhittaker25 Aug 23 '17 at 15:11

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.