Is this a proper way to use a class in PHP?

class MyImageTestClass{  
    var $url = "nfl2.jpg";  
    var $ID = "one";  

    function __construct() {  

    }  

    function setUrl($value){  
        $this->url = $value;  
    }  

    function setID($newid){  
        $this->ID = $newid ;  
    }  

    function loadImg($value,$newid){  
        $this->setID($newid);  
        $this->setUrl($value);  
        print "<img id=\"$this->ID\" src=\"$this-&g开发者_如何学运维t;url\"/>";  
    }  
}  

$n = new MyImageTestClass();  

$n->loadImg("peterson.jpg","two");  

Or should I keep the setID() and setURL out of the loadImg function and then write,

$n->setID();  
$n->setUrl();  
$n->loadImg();  

---

I would use "private" or "protected" or "public" instead of "var". Also, I'd set the defaults in the constructor method after I declared the properties as private etc above.

Personally, this is how I would do it, but It's been a year or two since I've done PHP...

class ImageBuilder{  
    private $url;    
    private $ID;    

    function __construct($url, $ID) {  
        $this->setUrl($url); 
        $this->setID($ID);   
    }  

    function setUrl($url){  
        if($url == NULL){
            $this->url = "nfl2.jpg";
        }else{
            $url = htmlentities($url, ENT_QUOTES);
            $this->url = $url;
        }
    }  

    function setID($ID){  
        if($ID == NULL){  
            $this->ID = "one";  
        }else{
            $this->ID = $ID;  
        }   
    }  

    function setImg($url, $ID){  
        $this->setUrl($url);
        $this->setIId($ID);
    }   

    function printImg(){  
        print "<img id=\"{$this->ID}\" src=\"{$this->url}\"/>";  
    }  
}  

$n = new ImageBuilder("peterson.jpg", "two");  

$n->printImg();

Also, if you wanted to use in your database loop:

//pretend this is coming out of your DB
$db_img = array(array("url" => "url.png", "id" => "1"), array("url" => "blah.gif", "id" => "2"));

$image = new ImageBuilder();

foreach($db_img as $img){
    $image->setImg($img["url"], $img["id"]);
    $image->printImg();        
} 

---

You can go for either way; the main thing is that all your functions remain inside the class itself. So there is no problem with any way you go with.

But from your code, i suspect you need just one function:

function loadImg($value,$newid){
      print "<img id=\"$newid\" src=\"$value\"/>";
}

No need for other functions.

---

What I meant is that you should use a templating system so as to separate the preparation of output data from its visualization.

$data = $myLibrary->getSomeData($arguments);
$t = new SomeTemplatingEngine;
$t->setData($data);
$t->setTemplate('someTemplate.html');
return $t->renderToHtml();

As opposed to:

$r = mysql_query(...)
while ($row = mysql_fetch_assoc($r))
{
  print "<tr>";
  print "<td>";
  print $row['someField'];
  ...
}

My favourite template engine is Dwoo, try it.

---

1. As a rule of thumb, you should never embed HTML in executable code. You probably would be better off with a templating system.
2. If you want to explicitly declare getter/setter methods, it makes sense to make the corresponding properties protected. Otherwise there's nothing to stop you or anyone else to skip the methods and read/write the properties directly.

3. What if the value passed to setUrl() contains unsanitized input? Every dynamic value that's rendered should be fed to htmlentities() or such. Imagine:

   setUrl('http://somesite.org/dir/?param1=<script>alert("I CAN HAS HACK");</script>");

---

In this particular case you have an Immutable object which means that you can remove setters and use the following version:

class MyImageTestClass{  
    private $_url;  
    private $_id;  

    function __construct($id, $url) {  
        $this->_id = $id;
        $this->_url = $url;
    }  

    function loadImg(){  
        return "<img id=\"" . $this->_id . "\" src=\"" . $this->_url . "\"/>";  
    }  
}  

$n = new MyImageTestClass("foo", "bar.jpg");  

echo $n->loadImg();