Nay wrote:
I hope this gives you a clearer view of what I'm doing (as well as my ametuer OOP

). I'd welcome advice on my current situation as well as any flaw or bad pratice you see in my code.
-Nay
First, some refactoring to disentangle various threads. Two aims:
(1) separate out discrete tasks into individual functions
(2) separate business logic (model) from presentation logic (view)
The classes below cover the biz logic only.
Not tested and may have bugs. The point is just to give you some ideas about how to structure a class. The general principle is that each function - or class - should do just one thing.
I haven't been consistent with var names - some camel back (as you have written) but I couldn't stop myself writing some underscore-separated. This is one of those personal preference things: I always write vars the latter way but fn names the former.
Code: Select all
<?php
$this->error_report; // a var less likely to be confused with..
$this->errorReport(); // a method
?>
The way private & public methods are separated below is again just my own way of working. Private method names always start with an underscore and there's a dirty great comment block separating the two groups. I find it helps to highlight the class interface (ie the public methods). In php there is no difference between private & public of course, hence there is possibly a need to do something of this kind.
Code: Select all
<?php
/*
CLASS PageVars
No methods - just properties. Putting page vars in their own class helps
to focus on what is actually to be output on the page by providing an
at-a-glance list. This gets more useful the more complicated is the
model. (I hope I didn't miss any page vars in your original script).
Not a standard OOP practice or anything just something I like to do.
This is one of the very few cases where I wouldn't bother with getters
and setters for class properties.
*/
class PageVars
{
var $hits_today = 0;
var $total_hits = null;
var $message = null;
var $date = null;
var $row_vars = null;
var $log_links = null;
}
///////////////
// END CLASS //
///////////////
/*
CLASS: HTML
I don't know what was in the previous Config class - I'm guessing you
don't really need to extend a base class.
The class name probably ought to be changed from "HTML" to
LogReport or something along those lines. Naming is very important
once you start encapsulating. The whole point is to hide a bunch of
functionality behind an interface: a good name explains what's under
the bonnet without having to look, a bad one leaves you wondering
what's going on.
*/
Class HTML
{
var $log_folder = 'path/to/file.php'; // or pass var to constructor if not constant
var $max_log_links = 20; // or whatever
var $date_format = 'd-m-Y';
/*
param (array) $resultArray
*/
function HTML($resultArray)
{
$this->resultArray = array_reverse($resultArray); #?
$this->page_vars =& new PageVars;
}
function setPageVars()
{
$this->page_vars->hits_today = count($this->resultArray);
$this->page_vars->total_hits = $this->_totalHits();
$this->page_vars->message = $this->_message();
$this->page_vars->date = $this->_date();
$this->page_vars->row_vars = $this->_rows();
$this->page_vars->log_links = $this->_logLinks();
}
function &getPageVars()
{
return $this->page_vars;
}
//////////////////////////////////////////
// PRIVATE //
//////////////////////////////////////////
/*
return (integer)
*/
function _totalHits()
{
$total_hits = 0;
$dh = opendir($this->logFolder);
# replaced: while($file = readdir($dh)) - see php manual
while (false !== ($file = readdir($dh)))
{
if(is_file($this->logFolder . '/' . $file))
{
$total_hits += count(file($this->logFolder . '/' . $file));
}
}
return $total_hits;
}
/*
html formatting (the span tag) has been removed from the message
return (string)
*/
function _message()
{
if(empty($this->resultArray))
{
$message = 'No logs found for today (';
$message .= date($this->date_format);
} else {
$message = null;
}
return $message;
}
/*
return (string)
*/
function _date()
{
if(isset($_GET['date']))
{
$date = $_GET['date'][0] . $_GET['date'][1] . '-';
$date .= $_GET['date'][2] . $_GET['date'][3] . '-';
$date .= $_GET['date'][6] . $_GET['date'][7];
} else {
$date = date($this->date_format);
}
return $date;
}
/*
return (array)
*/
function _rows()
{
$rows = array();
$row =& new Row($this->time_format);
for($i = 0; $i < count($this->resultArray); $i++)
{
# $logString isn't a string - it's an array - so let's call it $log to avoid confusion
$log = explode(' | ', $this->resultArray[$i]);
$rows[$i] = $row->getRow($log, $i);
}
return $rows;
}
function _logLinks()
{
$ob =& new LogLinks($this->log_folder, $this->max_log_links);
$ob->setLinks();
return $ob->getLinks();
}
}
///////////////
// END CLASS //
///////////////
/*
CLASS Row
Defines page vars for the current table row.
These methods work together and probably deserve a class of their
own - also LogLinks class.
*/
class Row
{
/*
param (string) $time_format
*/
function Row($time_format)
{
$this->time_format = $time_format;
}
/*
param (array) $log
param (integer) $i
return (array)
*/
function getRow($log, $i)
{
$row = array();
$row['name'] = $this->_name($log[0]);
$row['time'] = date($this->timeFormat, $log[1]);
$row['ip'] = $log[2];
$row['res'] = $log[3];
$row['browser'] = $log[4];
return $row;
}
//////////////////////////////////////////
// PRIVATE //
//////////////////////////////////////////
/*
Frequently there are if tests before setting a var. It's usually best to
refactor the logical test and the actions to be performed in each case
into separate functions (unless it's something trivial like
$name = 'Guest' which doesn't need it's own fn). Fns should do just
one thing: the logical test is obscured if you add in the actions in the
same fn. Another method _hyperlinkName carries out the task of
setting the hyperlink.
param (string) $name
return (string)
*/
function _name($name)
{
if($name == 'xanga')
{
return 'Guest';
} else {
return $this->_hyperlinkName($name);
}
}
/*
param (string) $name
return (string)
*/
function _hyperlinkName($name)
{
$link = '<a href="http://www.xanga.com/home.aspx?user=';
$link .= $name;
$link .= '" target="_blank">';
$link .= $name;
$link .= '</a>';
return $link;
}
}
///////////////
// END CLASS //
///////////////
/*
CLASS LogLinks
Links to log reports?
*/
class LogLinks
{
// public
var links = '';
function LogLinks($logFolder, $max)
{
$this->logFolder = $logFolder;
$this->max = $max;
}
function setLinks()
{
$counter = 0;
$dh = opendir($this->logFolder);
# replaced: while($file = readdir($dh)) - see php manual
while (false !== ($file = readdir($dh)))
{
if(!is_file($this->logFolder . '/' . $file))
{
break;
}
if($counter > $this->max)
{
return;
}
$this->links .= $this->_logLink($file);
$counter++;
}
}
/*
return (string)
*/
function getLinks()
{
return $this->links;
}
//////////////////////////////////////////
// PRIVATE //
//////////////////////////////////////////
function _logLink($file)
{
$date = str_replace('.txt', '', $file);
$linkDate = $date[0] . $date[1] . '-';
$linkDate .= $date[2] . $date[3] . '-';
$linkDate .= $date[6] . $date[7];
return '<a href="index.php?date=' . $date . '">' . $linkDate . "</a>\n";
}
}
///////////////
// END CLASS //
///////////////
/*
In use:
*/
$report_page =& new HTML($resultArray);
$report_page->setPageVars();
$page_vars =& $report_page->getPageVars();
?>
That takes care of the business logic or model. The model output should be a bunch of format-free vars (let's just gloss over the hyperlinks and date formatting for now...).
Next, the view where formatting is applied to the model and the page is printed. You could include an html template with embedded echo calls such as:
Code: Select all
&lt;?php echo $page_vars-&gt;log_links; ?&gt;
You'll need a presentation function to loop through $row_vars array, printing one row at a time. In the template just call the fn and pass the $row_vars:
Code: Select all
&lt;?php echo printTableRows($page_vars-&gt;row_vars); ?&gt;
Embedded echo's & looping fns for data rows creates a very simple (but perfectly respectable) templating system. However, with a separate model (all the code which defines the unformatted page vars) and view (html templates & presentation fns), you can easily change to another system. The same model output (the $page_vars class in this case) can be packed off to any kind of view. Code has become more modular. It's easier to read because different threads have been disentangled. Html designers can now work on the html templates without having to read through any php files - and there's no html obscuring what's going on in the model code. If you ever want pdf or xml pages, you can easily create the appropriate templates and re-use the same model.
This doesn't answer your original question but, before you can consider how to combine classes in an OOP design, you have to figure out the component classes. This is usually quite a fluid stage where ideas can change as you think about the problem so it's often best not to write any code at all - simply define some interfaces. A UML diagram (see above link) can be a good way to start.