Please critique this code

eruna · May 27, 2011, 12:59am

Below is a script that extracts information from embed codes for Vimeo and Youtube. It returns a thumbnail, video with customized skin, or the movie ID.

I’m new to OOP and I’m unclear on storing and returning the results.
At the end of each function it has something like:


$this->Id=$id;
return $id;

It seems redundant, but I want the function to work Independently as well as be able to work with other functions.

Also is there a more elegant way to extract the id from the Youtube Embed code?

Any other feedback is also welcome.

Feel free to use this if it is helpful to you.

Thank You E.

eruna · May 27, 2011, 1:32am

The Code


<?php

/* usage

require('embed_code_extraction.php');
$embed_extraction = new EmbedCodeExtraction();
$embed_extraction->EmbedCode=$media['video'];
$embed_extraction->VideoHeight=250;
$embed_extraction->VideoWidth=350;
$thumbnail=$embed_extraction->get_embed_thumbnail();
$video_id=$embed_extraction->get_embed_id();
$video=$embed_extraction->get_embed_video()

*/

class EmbedCodeExtraction{

	public $EmbedCode;
	public $Id;
	public $Thumbnail;
	public $Video;
	public $EmbedOutput;
	public $EmbedType;	
	public $VideoHeight=250;
	public $VideoWidth=350;
	
	function get_embed_id(){
		
		$type=$this->get_embed_type();
		
		if($type=='vimeo'){
			$id=$this->find_vimeo_id();
		}
		
		else if( $type=='youtube'){
			$id=$this->find_youtube_id();
		}
				
		$this->Id=$id;
		return $id;
		
	}
	
	function get_embed_thumbnail(){
		
		$type=$this->get_embed_type();
				
		if($type=='vimeo'){
			$id=$this->find_vimeo_id();
			$thumbnail=$this->get_vimeo_thumbnail();
		}
		
		else if( $type=='youtube'){
			$id=$this->find_youtube_id();
			$thumbnail=$this->get_youtube_thumbnail();
		}
		
		$this->Thumbnail=$thumbnail;
		return $thumbnail;
		
	}
	
	
	function get_embed_video(){
		
		$type=$this->get_embed_type();
				
		if($type=='vimeo'){
			$Id=$this->find_vimeo_id();
			$video=$this->write_vimeo_video();
		}
		
		else if( $type=='youtube'){
			$Id=$this->find_youtube_id();
			$video=$this->write_youtube_video();
		}
		
		return $video;
		
	}
	
	//identify YouTube or Vimeo
	function get_embed_type(){

		$embed=$this->EmbedCode;
		
		if(strpos($embed,'vimeo')>0 ){ $type='vimeo'; }
		else if (strpos($embed,'youtube')>0 ){ $type='youtube'; }
		else { $type='unknown'; }
		
		$this->EmbedType=$type;
		return $type;
		
	}
	
	/* VIMEO EXTRACTION */
	function get_vimeo_thumbnail(){

		$id=$this->find_vimeo_id($embed);
			
		if($id){
			$hash = unserialize(file_get_contents("http://vimeo.com/api/v2/video/$id.php"));
			$thumbnail= $hash[0]['thumbnail_small'];
			$this->Thumbnail=$thumbnail;
			return $thumbnail;
		}
				
	}
	
	function write_vimeo_video(){
 
		$id=$this->find_vimeo_id();
		$h=	$this-> VideoHeight;
		$w= $this-> VideoWidth;
			
		if($id){
			$movie='<iframe src="http://player.vimeo.com/video/'.$id.'?title=0&amp;byline=0&amp;portrait=0&amp;color=E68C35" width="'.$w.'" height="'.$h.'" frameborder="0"></iframe>';
			
			return $movie;
		}
				
	}


	function find_vimeo_id(){
			$embed=$this->EmbedCode;

		if(strpos($embed,'vimeo')>0 && strpos($embed,'clip_id=')>0 ){
			$start=strpos($embed,'clip_id=')+8;
			$end=strpos($embed,'&',$start);
			$stop=$end-$start;
			$id=substr( $embed, $start,$stop);
		}
		else if(strpos($embed,'vimeo')>0 && strpos($embed,'http://player.vimeo.com/video/')>0){
			$start=strpos($embed,'/video/')+7;
			$end=strpos($embed,'?',$start);
			$stop=$end-$start;
			$id=substr( $embed, $start,$stop);
		}

				
		$this->Id=$id;
		return $id;

	}
	
	/* END VIMEO EXTRACTION */
	
	/* YOUTUBE EXTRACTION */
	
	function get_youtube_thumbnail(){

		$id=$this->Id;
			
		if($id){
			$thumbnail= "http://i2.ytimg.com/vi/$id/default.jpg";
			$this->Thumbnail=$thumbnail;
			return $thumbnail;
		}
				
	}
	
	function write_youtube_video(){
	
		$id=$this->find_youtube_id();
		$h=	$this-> VideoHeight;
		$w= $this-> VideoWidth;
		
		$video='
		<object width= "'.$w.'" height="'.$h.'">

			<param name="movie" value="http://www.youtube.com/v/'.$id.'&autoplay=0&rel=0&fs=1&color1=0x4E3400&color2=0x322A22&border=0&loop=0"></param>

			<param name="allowFullScreen" value="true"></param>

			<embed src="http://www.youtube.com/v/'.$id.'&autoplay=0&rel=0&fs=1&color1=0x4E3400&color2=0x322A22&border=0&loop=0" type="application/x-shockwave-flash" allowfullscreen="true" width="'.$w.'" height="'.$h.'"></embed>

		</object>
		';
		
		$this->Video=$video;
		return $video;
	
	}
	
	/* 

	EXAMPLE YOUTUBE VARIATIONS 

	[YouTube - &#x202a;Bolsa de Bota Atelier Favela&#x202c;&rlm;](http://www.youtube.com/watch?v=FQDgVkasz7o)

	<object width="425" height="344"><embed src="http://www.youtube.com/v/4Dq8dwyV35k?hl=en&fs=1" type="application/x-shockwave-flash" allowscriptaccess="always" allowfullscreen="true" width="425" height="344"></embed></object>

	<object width=""425"" height=""344"" data=""http://www.youtube.com/v/wPsru6TlgfI%26hl=en%26fs=1%26rel=0%26ap=%2526fmt=18"" type=""application/x-shockwave-flash"">
	<param name=""src"" value=""http://www.youtube.com/v/wPsru6TlgfI%26hl=en%26fs=1%26rel=0%26ap=%2526fmt=18"" />
	</object>

	<iframe title="YouTube video player" width="640" height="390" src="http://www.youtube.com/embed/VvC5HfUqa28" frameborder="0" allowfullscreen></iframe>

	<iframe title="YouTube video player" width="640" height="390" src="http://www.youtube.com/embed/taFNdDs1SVU?rel=0" frameborder="0" allowfullscreen></iframe>

	*/

	function find_youtube_id(){
		
		$embed=$this->EmbedCode;
	
		if(strpos($embed,'watch?v=')>0 ){
			$start=strpos($embed,'watch?v=')+8;
			$id=substr( $embed, $start);
		}
		else if(strpos($embed,'youtube.com/v/')>0){
			$start=strpos($embed,'/v/')+3;
			$end=strpos($embed,'?',$start);
			if(strpos($embed,'%')>0 && !$end){
				$end=strpos($embed,'%',$start);
			}
			else if(strpos($embed,'"' )>0 && !$end){
				$end=strpos($embed,'"',$start);
				
			}
			else if(strpos($embed,"'")>0 && !$end) {
				$end=strpos($embed,'\\'',$start);
			}
			$stop=$end-$start;
			$id=substr( $embed, $start,$stop);
		}
		
		else if(strpos($embed,'/embed/')>0){
		
			$start=strpos($embed,'/embed/')+7;
			$end=strpos($embed,'?',$start);

			if(strpos($embed,'%')>0 && !$end){
				$end=strpos($embed,'%',$start);
			}
			else if(strpos($embed,'"' )>0 && !$end){
				$end=strpos($embed,'"',$start);
				
			}
			else if(strpos($embed,"'")>0 && !$end) {
				$end=strpos($embed,'\\'',$start);
			}
			$stop=$end-$start;
			$id=substr( $embed, $start,$stop);
		}
		
		$id=trim($id);
		
		$this->Id=$id;
		return $id;

	}


	/* END YOUTUBE EXTRACTION */

} //end class

?>

tmapm · May 27, 2011, 5:20pm

I didn’t look through all of it, but You could easily rewrite this:


    function get_embed_id(){
       
        $type=$this->get_embed_type();
       
        if($type=='vimeo'){
            $id=$this->find_vimeo_id();
        }
       
        else if( $type=='youtube'){
            $id=$this->find_youtube_id();
        }
               
        $this->Id=$id;
        return $id;
       
    }

Into:


    function get_embed_id(){
       
        $type = $this->get_embed_type();
       
        if($type == 'vimeo'){
            $this->Id = $this->find_vimeo_id();
        }
       
        else if( $type == 'youtube'){
            $this->Id = $this->find_youtube_id();
        }
               
        return $this->Id;
       
    }

Or even something like…


    function get_embed_id(){
       
        /* Creates find_vimeo_id or find_youtube_id */
        $method = 'find_' . $this->get_embed_type() . '_id';
        
        // If this function exists.. run it
           if( method_exists($this, $method)) {
          $this->Id = call_user_func(array($this, $method));
        }

        return $this->Id;
       
    }

eruna · May 27, 2011, 6:57pm

Combining the two statements makes a lot of sense.
return $this->Id;

Your second variation is interesting to see, but I think it obscures the logic and doesn’t seem like it reduces the code much.

Thank You E!

rpkamp · May 27, 2011, 9:27pm

I have a few questions:

Why are all properties of the class public?
Why didn’t you make it a normal class (as opposed to using the singleton pattern)?

And I don’t want to a party pooper here, but is there any reason you didn’t use oEmbed ? Makes life a lot easier

eruna · May 27, 2011, 10:28pm

Looks like I could make these properties private because they are only defined by the script. correct?


    public $Id;
    public $Thumbnail;
    public $Video;
    public $EmbedOutput;
    public $EmbedType;

What’s a Singleton Pattern? I googled it but didn’t understand the results.
Is there a simple way to describe your comment?

Many thanks E.

rpkamp · May 27, 2011, 10:42pm

Your usage example right now is :


require('embed_code_extraction.php');
$embed_extraction = new EmbedCodeExtraction();
$embed_extraction->EmbedCode=$media['video'];
$embed_extraction->VideoHeight=250;
$embed_extraction->VideoWidth=350;
$thumbnail=$embed_extraction->get_embed_thumbnail();
$video_id=$embed_extraction->get_embed_id();
$video=$embed_extraction->get_embed_video()

With the singleton pattern it would become:


require('embed_code_extraction.php');
$thumbnail=EmbedCodeExtraction::get_embed_thumbnail($media['video']);
$video=$embed_extraction->get_embed_video($media['video'], 350, 250);

Of course you would then have to change the code to be able to work like this, but from what I can see this way is a lot cleaner because the way you’ve set up now you’re meddling with the internals of a class from outside the class, and that’s never a good thing.

Another way to go about it would be to say that you will only ever have one video object at a time, and if there are multiple videos at one page you’ll just use that same object over and over again but you’ll never have multiple instances of that one class at any one time, which is a strong indicator that you might use the singleton pattern instead.

It’s kind of a tough subject and I’m not sure if I’m explaining this well. If not please let me know.

Also, have you looked at oEmbed?

eruna · May 28, 2011, 6:11pm

Thanks for explaining this. oEmbed looks good. Though, what I wrote is working well.

E

rpkamp · May 28, 2011, 7:08pm

Yes apart from the structure I don’t really agree with, the functions themselves look good. Very neat and lean, I like it. For now I’d indeed go with this, but it might be an idea to switch to oEmbed if you ever need to support more providers than just youtube and vimeo

eruna · May 29, 2011, 3:32pm

Thank you Skallio for your feedback. I’ll have to do more reading to really understand what you are saying. I’ll keep oEmbed that in mind. For now those are the only two formats that have been used on the site.

E

kyberfabrikken · May 30, 2011, 9:02am

Why are you assigning the values to a property at all? Since it’s calculated when calling the method (function), there is no reason to store it.

eruna · May 30, 2011, 5:57pm

That’s a good point.

These variables could just be returned and not stored:

public $Thumbnail;
public $Video;
public $EmbedOutput;

Whereas these ones are reused:

private $Id;
private $EmbedType; 

public $EmbedCode;
public $VideoHeight=250;
public $VideoWidth=350;