Class critique please..

Not for 'how-to' coding questions but PHP theory instead, this forum is here for those of us who wish to learn about design aspects of programming with PHP.

Moderator: General Moderators

Post Reply
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Class critique please..

Post by Jenk »

Created this class as part of an FTP over HTTP type web-app.. but I'm not happy that the class is doing that much directly with the 'output' however, short of reading the file contents into a string (some files are 300mB+) I'm not sure what else can be done?

note: optinal arg $download, is to read the file without serving it.

Comments to be added when I can be bothered to :P

The method downloadFile is of course the cause of concern.

Code: Select all

<?php

class jmt_Download
{
	private $view;
	private $size;
	private $path;
	private $type;
	private $name;
	
	public function __construct ($view, $file, $folder, $download = null)
	{
		if (!file_exists($fullpath = realpath($folder . '/' . $file))) {
			throw new FileNotFoundException ('File path given does not exist.');
			return;
		} 
		
		$this->view = $view;
		$this->path = $fullpath;
		$this->name = $file;
		
		$this->readProperties();
		
		if (is_null($download)) {
			$this->downloadFile();
		} else {
			$this->view->setTemplate('fileProperties.tpl');
			$this->view->assignVariable('fileSize', $this->size);
		}
	}
	
	private function readProperties ()
	{
		if (function_exists('mime_content_type')) {
			$this->type = mime_content_type($this->path);
			$this->view->assignVariable('mimetype', $this->type);
		}
		
		$this->size = filesize($this->path);
	}
	
	private function downloadFile ()
	{
		header('Content-disposition: attachment; filename=' . $this->name . ';');
		header('Content-Length: ' . $this->size);
		readfile($this->path);
	}
	
	public function getSize ()
	{
		return $this->size;
	}
	
	public function getType ()
	{
		return $this->type;
	}
}

?>
User avatar
Weirdan
Moderator
Posts: 5978
Joined: Mon Nov 03, 2003 6:13 pm
Location: Odessa, Ukraine

Post by Weirdan »

I'm not sure what else can be done?
Either you pass the file through your script or redirect user to the file using Location: ...
Of course you can move downloadFile method to your Response class if that will make you happy :)
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

I know it can be passed/achieved technically, but it's the lack of seperation between logic and view within this class that is bugging me :)

However, creating an extra method in my view class just for this seems iffy too.
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post by Christopher »

I agree with Weirdan that it does not feel right. I would suggest three classes. A File class that can read the contents and hold the MIME type. A Download class that takes a File object or just a buffer and sets the headers appropriately. And finally your View class.

But this code belongs in your Controller:

Code: Select all

if (is_null($download)) {
                        $this->downloadFile();
                } else {
                        $this->view->setTemplate('fileProperties.tpl');
                        $this->view->assignVariable('fileSize', $this->size);
                }
(#10850)
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

I did originally have a File class to maintain the properties of the file, including the content of the file itself (but as mentioned previously, some files will be huge - much memory to be eaten so don't want to store them.)

So the structure would be some thing like (from the page controller level):

Code: Select all

<?php

include 'config/jmt_config.php';

$view = new jmt_View(new Smarty);

switch ($_GET['action']) {
	
	case 'download':
		if (!empty($_GET['file'])) {
			try {
				$file = new jmt_File($_GET['file'], $_SESSION['folder']);
				$class = new jmt_Download($view, $file);
				break;
			} catch (FileNotFoundException $e) {
				die('Error: Invalid file download');
			}
		}
	
	//other actions go here..
	
	default:
		die ('Invalid page');
		
}

$view->displayPage();

?>
Where by the jmt_File class does everything you see in jmt_Download above that involves reading the file and various properties. jmt_Download would literally just serve the file, i.e. the rest of what you see above.

Would this be the better process?

Aborint - RE: Bits that should be in controller.

Those variables are unique to that class (including template), should they really be in the controller?
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post by Christopher »

Maybe I am misunderstanding the difference between view and download, but shouldn't it be like this:

Code: Select all

<?php

include 'config/jmt_config.php';

$file = new jmt_File($_GET['file'], $_SESSION['folder']);

switch ($_GET['action']) {
	
	case 'download':
		if (!empty($_GET['file'])) {
			try {
				$class = new jmt_Download($file);
				break;
			} catch (FileNotFoundException $e) {
				die('Error: Invalid file download');
			}
		}
		break;
	
	case 'view':
		$view = new jmt_View(new Smarty, $file);
		$view->displayPage();
		break;

	default:
		die ('Invalid page');
		
}

?>
The common thing is the file -- which is the Model in this case I suppose. There are two different views of it.
(#10850)
User avatar
Jenk
DevNet Master
Posts: 3587
Joined: Mon Sep 19, 2005 6:24 am
Location: London

Post by Jenk »

Sorry, my bad explanation.

jmt_File at the time of posting this thread, does not exist :) (I was brain_dump()'ing when replying)

The application is a file browser, think ftp over http.

The actions include login/out, browsing files, administration, file download.

User browses to a directory and clicks the href for a file "index.php?action=download&file=blah.txt" (the directory path is stored within $_SESSION during browsing)

The get var 'file' will only be required when action is 'download'.

class jmt_Download is the logic for action 'download', jmt_View is simply a wrapper class for Smarty.

So it kind of works the other way round.. jmt_View object is passed around the logic, rather than the logic passed around the jmt_View object. Each logic class uses differing variables and templates etc, and sets them using jmt_View object methods.
Post Reply