using oop in php

Question

This is one of my first oop code in normal php without any framework. This a code that combines all css files into one and all js files into one and it works. Any suggestion on what can i make better in terms of oop or anything else

This is how it woeks .

i run http://localhost/folderName/css/styleindex.css

i have ht acess that passes everything to index.php(code below0.
here i extract type as css and page as styleindex by exploding styleindex.css
get the array of files that needs to be combined for this page and join the

disaplay this on browser with correct mime type and cache header

define('DEBUGMODE',01); 
class ResourceHandler
{
/**
 *  @var $FilesToParse files that needs to be parsed  
 */
    private $FilesToParse = array();

/**
 *  @var $type weather js or css 
 */ 
    private $type;


/**
 *  @var $page name/category of the page
 */
    private $page;


/**
 *  @var $AllowedTypes array of allowed types js or css 
 */
    private $AllowedTypes = array('css','js');


/**
 *  @var $CacheTime time for which cache is valid
 */
    private $CacheTime = 610000;




    function __construct($type, $page)
    {

        $this->setType($type);
        $this->setPage($page);

        header('ETag: Ei07072012745');
        //header('Content-type: text/'.$this->type." charset: UTF-8");
        if (substr_count($_SERVER['HTTP_ACCEPT_ENCODING'], 'gzip'))
        {
             //ob_start("ob_gzhandler"); 
        }
        else
        {
            ob_start();
        }           

        header ("Vary: negotiate,accept-language,accept-charset");
        header ("Cache-control: public");
        header('Content-Disposition: attachment; filename="'.$this->page.'.'.$this->type.'"');

        if(DEBUGMODE ==0)
        {
            $expire = "expires: " . gmdate("D, d M Y H:i:s", time() + $this->CacheTime) . " GMT";
            header ($expire);
        }
    }

    function setPage($PageName)
    {
        $Resource   = explode('.',$PageName);
        $this->page = $Resource[0];
    }


    function setType($type)
    {
        $type = strtolower($type);
        /*type needs to be js or css else dont proceed further*/
        if(!in_array($type,$this->AllowedTypes))
        {
            exit;
        }
        $this->type = $type;
    }

/**
 * Function to minify data
 * @return $FileContent  Minified file data 
 */ 
    private function minifyFiles($data)
    {
        /* remove comments */
        $data = preg_replace('!/\*[^*]*\*+([^/][^*]*\*+)*/!', '', $data);
        /* remove tabs, spaces, newlines, etc. */
        $data = str_replace(array("\r\n", "\r", "\n", "\t", '  ', '    ', '    '), '', $data);
        return $data;
    }

/**
 * Function to parse file and merge into one variable
 * @return $FileContent  Merged file data 
 */ 
    private function parseFiles()
    {
        $FileContent = '' ;
        foreach($this->FilesToParse as $File)
        {
            $FileContent = file_get_contents($File);
        }
        return $FileContent;
    }

    function __destruct()
    {
        ob_end_flush();
    }


/**
 * Function to display  css file
 * @return $output returns the combined, compresssed and gzipped data
 */ 
    private function css()
    {
        switch($this->page)
        {
            case 'index' :  $this->FilesToParse = array('style12.css','jquery.autocomplete.css');
                            break;
            DEFAULT      :  header($_SERVER["SERVER_PROTOCOL"]." 404 Not Found"); 
                            exit;
        }

        $data =  $this->parseFiles('css');  
        if(DEBUGMODE ==0)
        {
            $data = $this->minifyFiles($data);
        }
        return $data;
    } 


/**
 * Function to display js file
 * @return $output returns the combined and gzipped data
 */     
    private function js()
    {
        switch($this->page)
        {
            case 'index' :  $this->FilesToParse = array('jquery-1.6.2.js','jquery.autocomplete.js','facebox.js');
                            break;
            default      :  header($_SERVER["SERVER_PROTOCOL"]." 404 Not Found"); 
                            exit;
        }   
        $data =  $this->parseFiles('js');
        return $data;
    }

    function load()
    {
        $type = $this->type;
        $cwd  = getcwd();
        chdir($type);
        echo $this->$type(); 
        chdir($cwd);
    }


}

  $request  = explode("/resources/", $_SERVER['REQUEST_URI']); 
  $params   = array_filter(explode("/", $request[1]));  
  if(isset($params[0]) && isset($params[1]))
  {

    $resource = new ResourceHandler($params[0],$params[1]);
    $resource->load();
  }
  else 
  {
      header($_SERVER["SERVER_PROTOCOL"]." 404 Not Found"); 
  }
?>

cspray · Answer 1 · 2012-02-08 12:56:27Z

The first thing I notice is private everything. Generally I avoid private unless the function is really, really, really specific to only, and ever only, that class. This class could be extended but wouldn't really provide the extending class with anything but the public methods already available. Make them protected. I understand the concern in not wanting extending code to change the way the class works, but that's kind of the point. Trust the developers extending your class to know why they're extending the class and give them the opportunity to do so.

A lot of your stuff is hard-coded. While this works now it means when you add/change a file you have to change the class itself. This, in combination with private everything, will force somebody to eventually break the Open/Closed principle of SOLID. Perhaps add some parameters to your load() that accept an array of files to add for each file type. This way your users can add their own files to the list of those included without having to actually go in and change your code.

You're calling header a lot all over the place, especially in your __construct(). I would try to consolidate your header() calls into a centralized method; they're pretty important and might be a future pain point as you hunt your class over for where the invalid header() call is coming from. I would maybe do something like...

protected $headers = array();

protected function setDefaultHeaders() {
    $this->headers[] = 'Whatever headers you want for every request.';
    // rinse, repeat
}

public function setOptionalHeader($header) {
    $this->headers[] = $header;
}

protected function sendHeaders() {
    foreach ($this->headers as $header) {
        header($header);
    }
}

Why? Well, I can get all of those header calls out of the __construct() and replace it with a $this->setDefaultHeaders(). Also, all of my headers are being sent by one method, sendHeaders(). When the pain comes I have a somewhat easier stack trace to follow back, it starts at one point and I know where that point is. I've found that in debugging finding a good starting point is the hardest thing. Finally, you also know that the likely error is coming from something not right in setOptionalHeader().

Your __construct() is probably doing a tad too much. Maybe it should set the default headers. But it probably shouldn't be starting output buffering, particularly since the output buffering only stops when the class is destroyed. This may wind up causing everything echod to be included in your CSS/JS minification output. Get rid of all the extra actions in the constructor; the constructor should try to be limited to only setting up the class state and data so other actions can do the heavy lifting.

I also don't like that you are only echoing output on __destruct(), ultimately this means you have little direct control of when your data gets output. You see, the output buffering will only ever stop when your class is destroyed. Since the garbage collector decides when that is you're leaving your output buffering going the entire request; it doesn't stop until the garbage collector runs AND determines the class is no longer being used. With something as important as CSS and JavaScript I would want that output ASAP and exactly when I say. Get rid of the __destruct() and output your data in its own function. Perhaps this function can also be what starts and ends output buffering, solving the OB problem mentioned above.

I really like most of your methods design though; the names are reasonably descriptive and, for the most part, they're sufficiently short and easy to read. There's some styling things I disagree with, for example if a variable is PascalCase I assume it to be an object and reserve camelCase for scalar types. Of course, that is just a picky personal preference and the important thing is to be consistent and follow whatever naming conventions strategies setup by your team/the project.

Welcome to PHP and OOP! I hope you enjoy it as much as I have. :)

Would the downvoter explain how my review could be improved?
thanks for such a long answer. I too taught headers needed to be separate. and thanks for the destructor point i never taught about it. Also thanks for the protected bit though no one will ever extend this.
@WebDeveloper Generally, protected should be the default protection level for your class members. Public members describe the API of your class (how other people can use the service your class provides) and private members are intended for very specific cases where the class MUST behave in a particular way that CANNOT be changed. Protected means that you can inherit from the class and change its behaviour without changing its public API (this is a huge simplification but should suffice as a general rule of thumb)

asked	1 year ago
viewed	243 times
active	1 year ago

using oop in php

1 Answer

Your Answer

Not the answer you're looking for? Browse other questions tagged php oop clean-code or ask your own question.

using oop in php

1 Answer

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged php oop clean-code or ask your own question.

Related