Page 1 of 2
refactoring 101
Posted: Thu Sep 06, 2007 5:06 pm
by thinsoldier
I had to quickly whip up something today for 2 areas of a site that were nearly identical.
1 area was for shipped items and one was for unshipped items.
I've been readling lately about OOP and how I should be maximizing code re-use etc and I think this is a good one to start on.
How wold you guys go about rewriting this?
Is your coding style vastly different from mine?
Would you actually use a framework that consists of dozens and dozens of files just to build this simple thing?
So please give me your suggestions and I'll try apply them to this and see if I can wind up with something easier to read, and with a hell of a lot less duplication.
The output can be seen here:
http://thinsoldier.com/wip/php/ref1.png
http://thinsoldier.com/wip/php/ref2.png
The only differences between the 2 files are on lines:
6
8
21
36
42
44
86
pending.php
Code: Select all
<?php
include_once('../scripts/php/config.php');
include_once('../scripts/php/admin.session.php');
if($cmd == 'markshipped')
{
$today = date("Y-m-d");
$sql = "UPDATE orders SET status='shipped',date_shipped=$today WHERE order_id=$orderID LIMIT 1";
mysql_query($sql);
}
if($cmd == 'markcancelled')
{
$sql = "UPDATE orders SET status='cancelled' WHERE order_id=$orderID LIMIT 1";
mysql_query($sql);
}
$H = new HTMLHead($ADMINHEAD);
$H->PrefixTitle("Pending Orders - ");
$H->LinkCSS('orders.css');
$H->ExtraCSS('#leftbar li#orders-sub, #leftbar li#orders-sub .sub-menu{display:block;}');
echo $H->Output();
?>
<div id="admin-container">
<?php
include('../includes/inc.admin_header.php');
include('../includes/inc.admin_navigation.php');
?>
<div id="content">
<h2>Pending Orders</h2>
<?
include_once('order_classes.php');
$sql = "SELECT * FROM orders WHERE status='pending' ORDER BY date_ordered DESC";
$pgn8 = new pagination($sql, 5, 'pgn8_pending_sql');
$pageResult = $pgn8->findPageResult();
while ($allorders = mysql_fetch_assoc($pageResult))
{
$o = new OrderInfo($allorders[order_id]);
$Orders[] = $o;
}
?>
<? echo $pgn8->showingFromTo();?>
<? echo $pgn8->displayPagination(); ?>
<table class="datatable">
<thead>
<th scope="col">Order</th>
<th scope="col">Purchased Items</th>
<th scope="col">Status</th>
<th></th>
<th></th>
<thead>
<?
foreach($Orders as $O)
{
echo '<tr>';
echo '<td>';
echo '<b>'.date(" F d, Y", strtotime($O->Order[date_ordered])).'</b><br />';
echo 'Order ID: '.str_replace('-','',$O->Order[date_ordered]).'-'.$O->Order['order_id'].'<br />';
echo 'Bill to: '.$O->Billing[card_name]. '<br />';
echo 'Ship to: '.$O->Shipping[name];
echo '</td><td>';
foreach($O->Purchased as $P)
{echo $P[name].' x '.$P[quantity].'<br />'; }
echo '<b>Subtotal: $'.number_format($O->Order[subtotal],2).'</b>';
echo '</td><td>';
echo ucwords($O->Order[status]);
echo '</td><td>';
echo '<ul>';
printf('<li><a href="%s?page=%s&cmd=markshipped&orderID=%s">Mark as Shipped</a></li>',thispage(),$page,$O->Order['order_id'] );
printf('<li><a href="%s?page=%s&cmd=markcancelled&orderID=%s">Cancel Order</a></li>',thispage(),$page,$O->Order['order_id'] );
echo '</ul>';
echo '</td><td>';
printf('<a class="view" href="order_view.php?page=%s&orderID=%s">View Details</a>',$page,$O->Order['order_id'] );
echo '</td></tr>';
echo '<tr><td colspan="5" class="xhrdetail"></td></tr>';
}
?>
</table>
<? echo $pgn8->displayPagination(); ?>
</div>
<?php
include('../includes/inc.admin_footer.php');
?>
</div>
</body>
</html>
shipped.php
Code: Select all
<?php
include_once('../scripts/php/config.php');
include_once('../scripts/php/admin.session.php');
if($cmd == 'markpending')
{
$sql = "UPDATE orders SET status='pending', date_shipped='' WHERE order_id=$orderID LIMIT 1";
mysql_query($sql);
}
$H = new HTMLHead($ADMINHEAD);
$H->PrefixTitle("Shipped Orders - ");
$H->LinkCSS('orders.css');
$H->ExtraCSS('#leftbar li#orders-sub, #leftbar li#orders-sub .sub-menu{display:block;}');
echo $H->Output();
?>
<div id="admin-container">
<?php
include('../includes/inc.admin_header.php');
include('../includes/inc.admin_navigation.php');
?>
<div id="content">
<h2>Shipped Orders</h2>
<?
include_once('order_classes.php');
$sql = "SELECT * FROM orders WHERE status='shipped' ORDER BY date_ordered DESC";
$pgn8 = new pagination($sql, 5, 'pgn8_shipped_sql');
$pageResult = $pgn8->findPageResult();
while ($allorders = mysql_fetch_assoc($pageResult))
{
$o = new OrderInfo($allorders[order_id]);
$Orders[] = $o;
}
?>
<? echo $pgn8->showingFromTo();?>
<? echo $pgn8->displayPagination(); ?>
<table class="datatable">
<thead>
<th scope="col">Order</th>
<th scope="col">Purchased Items</th>
<th scope="col">Status</th>
<th></th>
<th></th>
<thead>
<?
foreach($Orders as $O)
{
echo '<tr>';
echo '<td>';
echo '<b>'.date(" F d, Y", strtotime($O->Order[date_ordered])).'</b><br />';
echo 'Order ID: '.str_replace('-','',$O->Order[date_ordered]).'-'.$O->Order['order_id'].'<br />';
echo 'Bill to: '.$O->Billing[card_name]. '<br />';
echo 'Ship to: '.$O->Shipping[name];
echo '</td><td>';
foreach($O->Purchased as $P)
{echo $P[name].' x '.$P[quantity].'<br />'; }
echo '<b>Subtotal: $'.number_format($O->Order[subtotal],2).'</b>';
echo '</td><td>';
echo ucwords($O->Order[status]);
echo '</td><td>';
echo '<ul>';
printf('<li><a href="%s?page=%s&cmd=markpending&orderID=%s">Mark as Pending</a></li>',thispage(),$page,$O->Order['order_id'] );
//printf('<li><a href="%s?page=%s&cmd=markcancelled&orderID=%s">Cancel Order</a></li>',thispage(),$page,$O->Order['order_id'] );
echo '</ul>';
echo '</td><td>';
printf('<a class="view" href="order_view.php?page=%s&orderID=%s">View Details</a>',$page,$O->Order['order_id'] );
echo '</td></tr>';
echo '<tr><td colspan="5" class="xhrdetail"></td></tr>';
}
?>
</table>
<? echo $pgn8->displayPagination(); ?>
</div>
<?php
include('../includes/inc.admin_footer.php');
?>
</div>
</body>
</html>
Posted: Thu Sep 06, 2007 7:56 pm
by Christopher
I would expand your HTMLHead class into a full Response class to do the page building (e.g., footer too). Then I would encapsulate the database code into an OrdersModel class because there is a lot of commonality and it would be clearer for me to just see $model->markAsShipped();. Everything else left would be presentation code -- essentially a fancy template.
And yes, I would us a framework for this (as you are with you pagination class). This is about the level of complexity where using a framework pays off for me.
Posted: Thu Sep 06, 2007 9:36 pm
by thinsoldier
Where could I find a simple example of a "full Response class"?
"(as you are with you pagination class)" not sure I understand? My pagination is home-made
I'll give it a think and come up with some pseudo-code tomorrow to see if it looks more like what you're talking about.
Posted: Thu Sep 06, 2007 10:00 pm
by Christopher
thinsoldier wrote:Where could I find a simple example of a "full Response class"?
I think of a Response class as something that builds basic HTML structure, sets headers, encodings, <head> stuff, etc. and does redirects. It might deal with header and footer type layout HTML as well. You fill in the content.
thinsoldier wrote:"(as you are with you pagination class)" not sure I understand? My pagination is home-made

Home-made or no, if you code to pre-existing conventions and code libraries then you are using a framework.
thinsoldier wrote:I'll give it a think and come up with some pseudo-code tomorrow to see if it looks more like what you're talking about.
I am interested to see what you come up with.
Posted: Thu Sep 06, 2007 10:20 pm
by thinsoldier
ok, here's some make believe code
Code: Select all
<?php
include_once('../scripts/php/config.php');
// took all the site-wide configuration vars and turned them into a Class that I can extend for handling things specific to the public or administration sides of the site.
$CONF = new AdminConfig;
$CONF->SecureSession(); // replaces admin.session.php
include_once('order_classes.php');
$model = new OrderEntity();
if($cmd && $orderID)
{
$model->SetID($_GET['orderID']);
if($cmd == 'markpending'){$model->markAsPending();}
if($cmd == 'markshipped'){$model->markAsShipped();}
if($cmd == 'markcancelled'){$model->markAsCancelled();}
}
$model->GetAllOrdersByStatus();
$sql = $model->GetSqlUsed();
$pgn8 = new pagination($sql, 5, 'pgn8_pending_sql');
$page = new AdminTemplate($pgn8);
$page->UseTemplate($CONF->TemplateDir.'/orders_pending.php');
// the template assumes the existance of $pgn8 & $model
$page->SetTitle("Pending Orders - ".$CONF->CompanyName.' Administration');
$page->LinkCSS('orders.css');
$page->ExtraCSS('#leftbar li#orders-sub, #leftbar li#orders-sub .sub-menu{display:block;}');
echo $page->Output();
?>
Posted: Fri Sep 07, 2007 1:18 am
by Christopher
Two initial comments. First is the question of where validation should reside. It can either be in the controller code as you show, or if you don't want controller code then it could be moved to the model and you could do something like:
Code: Select all
$model->markAs($cmd, $_GET['orderID']);
Second is the small SQL dependency mess between the model and the pagination code. It is a difficult call as to whether it is better to make the pagination code a clear target to which things can provide necessary data, or whether to modify the paginator so that it can accept a model object that provides the necessary interface.
The code you post really presents the difficult design decisions that need to be made as you refactor code to clearer, cleaner dependencies. It also brings up the perennial question of how to apply the useful ideas in the MVC pattern, while at the same time being flexible to achieve the best design your your own needs and style. Interesting.
Posted: Fri Sep 07, 2007 2:46 pm
by thinsoldier
yea I don't know what to do with the pagination either.
How it works at the moment is I give it the sql string that I know will find all of what I want to find, a records per page limit (5 in this case) and finally a 'name' to give to a $_SESSION variable that will remember the sql string used to find all of what I want to find.
The pagination class takes the limit per page and calculates the limit string to add on to the end of the main sql that would have found everything ie $sql.'LIMIT 15, 20'.
This way I don't have to do a query that find everything and only return a few. Instead I only get what's in the LIMIT range. However, looking at the pseudo code, if I do it like how it's written above: $model->GetAllOrdersByStatus(); I would probably be performing a query in the method which finds everything only to turn around and use the sql from that method in the pagination class which will again perform another query but with a limit. So it kind of defeats the point of having $model->GetAllOrdersByStatus();
I'd be better off just typing out the sql to pass to $pgn8 and that way I won't have 2 queries happening when only one is needed. But at the same time I'm sure in some other section I'll find use for a quick and easy $model->GetAllOrdersByStatus();
As for $cmd, thats more for control flow than validation. On many layouts I've had various data related things happen based on $cmd value and also have very many visual aspects of the layout change based on the same $cmd value. So i figured it would be best to have it in the main file where it's immediately visible to see what role it plays in all the different areas.
Or should I completely rething my ideas of control flow?
Posted: Fri Sep 07, 2007 4:00 pm
by Christopher
The pagination thing is a difficult decision. I also pass base SQL to paginators and have it add the LIMIT. But you could also add a findLimited($start, $size) function to your model. That would probably change its interface around to have limitByStatus() style constraint methods, but that might be cleaner.
As to rethinking control flow, that may be a good idea -- just to try out other conventions. That's the glue so you might want to experiment a little to see of something else makes the overall picture cleaner.
Posted: Mon Sep 10, 2007 3:33 pm
by thinsoldier
Lol. Isn't that great.
google for "php control flow conventions" and this thread is one of the first results.
Posted: Fri Sep 14, 2007 10:54 am
by thinsoldier
So I've got one class that does everything needed to get the desired pages worth of records from the database and also update thier status value to shipped, cancelled, or pending. By default it does stuff for 'shipped' records.
For working with 'pending' records I extend that first class and changed the constructor to initially get orders that are 'pending' instead of 'shipped'.
Also changed the links that will be displayed for performing the action of making a pending record shipped and vice versa.
On the actual pages the user interacts with (OV_pending & OV_shipped) I only need to pass the <h2> value and the object to the OrderTemplate function to get the page layout.
Suggestions for improvement? What seems wrong? Do you see this way of doing things somehow more complicated or decreasing flexibility?
OV_pending.php
Code: Select all
<?php
include_once('../scripts/php/config.php');
include_once('../scripts/php/admin.session.php');
include_once('class.OV.php');
$o = new OrderView_Pending($cmd, $orderID);
OrderTemplate('Pending', $o);
?>
OV_shipped.php
Code: Select all
<?php
include_once('../scripts/php/config.php');
include_once('../scripts/php/admin.session.php');
include_once('class.OV.php');
$o = new OrderView($cmd, $orderID);
OrderTemplate('Shipped', $o);
?>
class.OV.php
Code: Select all
<?
/**
* Yea it's named orderView but I think it's more like a 'model'
* The OrderTemplate function at the end of this file is the actual 'view'
*/
class OrderView
{
private $pgn8;
private $Orders;
function __construct($cmd='', $orderID='')
{
$this->HandleCommand($cmd,$orderID);
$this->GetOrdersByStatus('shipped');
}
function HandleCommand($cmd, $orderID)
{
if($cmd == 'markpending')
{
$this->MarkAsPending($orderID);
$this->Redirect();
}
if($cmd == 'markshipped')
{
$this->MarkAsShipped($orderID);
$this->Redirect();
}
if($cmd == 'markcancelled')
{
$this->MarkAsCancelled($orderID);
$this->Redirect();
}
}
/**
* After performing one of the action commands the user would see the ugly query string in the URL
* this simply redirects the page back to the current page.php?page=3 after the command has been processed.
*/
function Redirect()
{
header("Location: ".thispage().'?page='.$_GET['page']);
}
function MarkAsPending($id)
{
$sql = "UPDATE orders SET status='pending', date_shipped='' WHERE order_id=$id LIMIT 1";
mysql_query($sql);
}
function MarkAsShipped($id)
{
$today = date("Y-m-d");
$sql = "UPDATE orders SET status='shipped',date_shipped=$today WHERE order_id=$id LIMIT 1";
mysql_query($sql);
}
function MarkAsCancelled($id)
{
$sql = "UPDATE orders SET status='cancelled', date_shipped='' WHERE order_id=$id LIMIT 1";
mysql_query($sql);
}
function GetOrdersByStatus($status='shipped')
{
include_once('order_classes.php');
// this contains the OrderInfo class that does all the hard work gathering detailed information
// about each order.
// Stuff like who it was shipped to, which products and how many were included in the order,
// the total cost of the order etc.
$sql = "SELECT * FROM orders WHERE status='$status' ORDER BY date_ordered DESC";
$this->pgn8 = new pagination($sql, 5, "pgn8_$status_sql");
$pageResult = $this->pgn8->findPageResult();
while ($allorders = mysql_fetch_assoc($pageResult))
{
$o = new OrderInfo($allorders[order_id]);
$Orders[] = $o;
}
$this->Orders = $Orders;
}
function ActionButtons($O)
{
echo '<ul>';
printf('<li><a href="%s?page=%s&cmd=markpending&orderID=%s">Mark as Pending</a></li>',thispage(),$_GET['page'],$O->Order['order_id'] );
echo '</ul>';
}
function Output()
{
echo $this->pgn8->showingFromTo();
echo $this->pgn8->displayPagination();
?>
<table class="datatable">
<thead>
<th scope="col">Order</th>
<th scope="col">Purchased Items</th>
<th scope="col">Status</th>
<th></th>
<th></th>
<thead>
<?
foreach($this->Orders as $O)
{
echo '<tr>';
echo '<td>';
echo '<b>'.date(" F d, Y", strtotime($O->Order[date_ordered])).'</b><br />';
echo 'Order ID: '.str_replace('-','',$O->Order[date_ordered]).'-'.$O->Order['order_id'].'<br />';
echo 'Bill to: '.$O->Billing[card_name]. '<br />';
echo 'Ship to: '.$O->Shipping[name];
echo '</td><td>';
foreach($O->Purchased as $P)
{echo $P[name].' x '.$P[quantity].'<br />'; }
echo '<b>Subtotal: $'.number_format($O->Order[subtotal],2).'</b>';
echo '</td><td>';
echo ucwords($O->Order[status]);
echo '</td><td>';
$this->ActionButtons($O);
echo '</td><td>';
printf('<a class="view" href="order_view.php?page=%s&orderID=%s">View Details</a>',$page,$O->Order['order_id'] );
echo '</td></tr>';
echo '<tr><td colspan="5" class="xhrdetail"></td></tr>';
}
echo '</table>';
echo $this->pgn8->displayPagination();
}
} // end Orderview class
/**
* Extends the previous class for use with pending orders
*/
class OrderView_Pending extends OrderView
{
function __construct($cmd='', $orderID='')
{
$this->HandleCommand($cmd,$orderID);
$this->GetOrdersByStatus('pending');
}
function ActionButtons($O)
{
echo '<ul>';
printf('<li><a href="%s?page=%s&cmd=markshipped&orderID=%s">Mark as Shipped</a></li>',thispage(),$_GET['page'],$O->Order['order_id'] );
printf('<li><a href="%s?page=%s&cmd=markcancelled&orderID=%s">Cancel Order</a></li>',thispage(),$_GET['page'],$O->Order['order_id'] );
echo '</ul>';
}
}
/**
* Takes one of the objects and displays it's output() within an page layout.
* If the layout had a lot more variables then I probably would have done it as a regular include
* that expects certain variables to already exist.
* I dont like having a dozen arguments in my function calls.
*/
function OrderTemplate($pOs, $modelOutput)
{
global $ADMINHEAD;
global $CONFIG;
$H = new HTMLHead($ADMINHEAD);
$H->PrefixTitle("Shipped Orders - ");
$H->LinkCSS('orders.css');
$H->ExtraCSS('#leftbar li#orders-sub, #leftbar li#orders-sub .sub-menu{display:block;}');
echo $H->Output();
?>
<div id="admin-container">
<?
include('../includes/inc.admin_header.php');
include('../includes/inc.admin_navigation.php');
?>
<div id="content">
<h2><?=$pOs?> Orders</h2>
<? echo $modelOutput->Output(); ?>
</div>
<? include('../includes/inc.admin_footer.php'); ?>
</div>
</body>
</html>
<?
}
?>
The resulting pages work exactly as they did before
http://thinsoldier.com/wip/php/ref1.png
http://thinsoldier.com/wip/php/ref2.png
Posted: Fri Sep 14, 2007 12:22 pm
by Christopher
Here is how I might separate things:
OV_pending.php
Code: Select all
<?php
include_once('../scripts/php/config.php');
include_once('../scripts/php/admin.session.php');
include_once('OrderModel.php');
$om = new OrderModel($cmd, $orderID);
$ov = new OrderView_Pending($om);
$ov->setTitle('Pending');
$t = new OrderTemplate($ov);
echo $t->render()
?>
OV_shipped.php
Code: Select all
<?php
include_once('../scripts/php/config.php');
include_once('../scripts/php/admin.session.php');
include_once('OrderModel.php');
$om = new OrderModel($cmd, $orderID);
$ov = new OrderView_Pending($om);
$ov->setTitle('Shipped');
$t = new OrderTemplate($ov);
echo $t->render()
?>
OrderModel.php
Code: Select all
<?
class OrderModel
{
private $pgn8;
private $Orders;
function __construct($cmd='', $orderID='')
{
$this->HandleCommand($cmd,$orderID);
$this->GetOrdersByStatus('shipped');
}
function HandleCommand($cmd, $orderID)
{
if($cmd == 'markpending')
{
$this->MarkAsPending($orderID);
$this->Redirect();
}
if($cmd == 'markshipped')
{
$this->MarkAsShipped($orderID);
$this->Redirect();
}
if($cmd == 'markcancelled')
{
$this->MarkAsCancelled($orderID);
$this->Redirect();
}
}
/**
* After performing one of the action commands the user would see the ugly query string in the URL
* this simply redirects the page back to the current page.php?page=3 after the command has been processed.
*/
function Redirect()
{
header("Location: ".thispage().'?page='.$_GET['page']);
}
function MarkAsPending($id)
{
$sql = "UPDATE orders SET status='pending', date_shipped='' WHERE order_id=$id LIMIT 1";
mysql_query($sql);
}
function MarkAsShipped($id)
{
$today = date("Y-m-d");
$sql = "UPDATE orders SET status='shipped',date_shipped=$today WHERE order_id=$id LIMIT 1";
mysql_query($sql);
}
function MarkAsCancelled($id)
{
$sql = "UPDATE orders SET status='cancelled', date_shipped='' WHERE order_id=$id LIMIT 1";
mysql_query($sql);
}
function GetOrdersByStatus($status='shipped')
{
include_once('order_classes.php');
// this contains the OrderInfo class that does all the hard work gathering detailed information
// about each order.
// Stuff like who it was shipped to, which products and how many were included in the order,
// the total cost of the order etc.
$sql = "SELECT * FROM orders WHERE status='$status' ORDER BY date_ordered DESC";
$this->pgn8 = new pagination($sql, 5, "pgn8_$status_sql");
$pageResult = $this->pgn8->findPageResult();
while ($allorders = mysql_fetch_assoc($pageResult))
{
$o = new OrderInfo($allorders[order_id]);
$Orders[] = $o;
}
$this->Orders = $Orders;
}
}
Code: Select all
class OrderView
{
private $model;
private $title = '';
function __construct($orders)
{
$this->Orders = $orders;
}
function setTitle($title)
{
$this->title= $title;
}
function getTitle($title)
{
return $this->title;
}
function ActionButtons($O)
{
echo '<ul>';
printf('<li><a href="%s?page=%s&cmd=markpending&orderID=%s">Mark as Pending</a></li>',thispage(),$_GET['page'],$O->Order['order_id'] );
echo '</ul>';
}
function Output()
{
echo $this->pgn8->showingFromTo();
echo $this->pgn8->displayPagination();
?>
<table class="datatable">
<thead>
<th scope="col">Order</th>
<th scope="col">Purchased Items</th>
<th scope="col">Status</th>
<th></th>
<th></th>
<thead>
<?
foreach($this->Orders as $O)
{
echo '<tr>';
echo '<td>';
echo '<b>'.date(" F d, Y", strtotime($O->Order[date_ordered])).'</b><br />';
echo 'Order ID: '.str_replace('-','',$O->Order[date_ordered]).'-'.$O->Order['order_id'].'<br />';
echo 'Bill to: '.$O->Billing[card_name]. '<br />';
echo 'Ship to: '.$O->Shipping[name];
echo '</td><td>';
foreach($O->Purchased as $P)
{echo $P[name].' x '.$P[quantity].'<br />'; }
echo '<b>Subtotal: $'.number_format($O->Order[subtotal],2).'</b>';
echo '</td><td>';
echo ucwords($O->Order[status]);
echo '</td><td>';
$this->ActionButtons($O);
echo '</td><td>';
printf('<a class="view" href="order_view.php?page=%s&orderID=%s">View Details</a>',$page,$O->Order['order_id'] );
echo '</td></tr>';
echo '<tr><td colspan="5" class="xhrdetail"></td></tr>';
}
echo '</table>';
echo $this->pgn8->displayPagination();
}
} // end Orderview class
class OrderView_Pending extends OrderView
{
function __construct($cmd='', $orderID='')
{
$this->HandleCommand($cmd,$orderID);
$this->GetOrdersByStatus('pending');
}
function ActionButtons($O)
{
echo '<ul>';
printf('<li><a href="%s?page=%s&cmd=markshipped&orderID=%s">Mark as Shipped</a></li>',thispage(),$_GET['page'],$O->Order['order_id'] );
printf('<li><a href="%s?page=%s&cmd=markcancelled&orderID=%s">Cancel Order</a></li>',thispage(),$_GET['page'],$O->Order['order_id'] );
echo '</ul>';
}
}
/**
* MAKE THIS A CLASS
*/
function OrderTemplate($modelOutput)
{
global $ADMINHEAD;
global $CONFIG;
$H = new HTMLHead($ADMINHEAD);
$H->PrefixTitle("Shipped Orders - ");
$H->LinkCSS('orders.css');
$H->ExtraCSS('#leftbar li#orders-sub, #leftbar li#orders-sub .sub-menu{display:block;}');
echo $H->Output();
?>
<div id="admin-container">
<?
include('../includes/inc.admin_header.php');
include('../includes/inc.admin_navigation.php');
?>
<div id="content">
<h2><?php echp $modelOutput->getTitle(); ?> Orders</h2>
<?php echo $modelOutput->Output(); ?>
</div>
<? include('../includes/inc.admin_footer.php'); ?>
</div>
</body>
</html>
<?
}
?>
Posted: Fri Sep 14, 2007 4:04 pm
by thinsoldier
yes that does make more sense separated.
A few issues though.
Why would pending.php and shipped.php both be using
$ov = new OrderView_Pending....
and...
why do OrderView_Pending and OrderModel have the same constructor?
Typo or was there a reason?
and at that point now I'm lost
...according to your pending.php and shipped.php the OrderView_Pending constructor should receive an OrderModel object as it's single argument.
...there's no class extended from OrderModel to handle pending records... I think the extended OrderView_Pending was supposed to extend....
ok been messing with it for a while. Trying to pass objects as arguments to methods in other objects seems tricky. Finally go it to work but had to make those private vars public for it to work.
here is code
pending.php
Code: Select all
<?php
include_once('../scripts/php/config.php');
include_once('../scripts/php/admin.session.php');
include_once('AR.OrderModel.php');
include_once('AR.OrderView.php');
include_once('AR.OrderTemplate.php');
$om = new OrderModel($cmd, $orderID);
$om->GetOrdersByStatus('pending');
$ov = new OrderView_Pending($om);
$t = new OrderTemplate($ov);
$t->Render();
?>
shipped.php
Code: Select all
<?php
include_once('../scripts/php/config.php');
include_once('../scripts/php/admin.session.php');
include_once('AR.OrderModel.php');
include_once('AR.OrderView.php');
include_once('AR.OrderTemplate.php');
$om = new OrderModel($cmd, $orderID);
$om->GetOrdersByStatus('shipped');
$ov = new OrderView($om);
$t = new OrderTemplate($ov);
$t->Render();
?>
OrderModel.php
Code: Select all
<?
class OrderModel
{
public $pgn8;
public $Orders;
function __construct($cmd='', $orderID='')
{
$this->HandleCommand($cmd,$orderID);
}
function HandleCommand($cmd, $orderID)
{
if($cmd == 'markpending')
{
$this->MarkAsPending($orderID);
$this->Redirect();
}
if($cmd == 'markshipped')
{
$this->MarkAsShipped($orderID);
$this->Redirect();
}
if($cmd == 'markcancelled')
{
$this->MarkAsCancelled($orderID);
$this->Redirect();
}
}
/**
* After performing one of the action commands the user would see
* the ugly query string in the URL
* this simply redirects the page back to the current page.php?page=3
* after the command has been processed.
*/
function Redirect()
{
header("Location: ".thispage().'?page='.$_GET['page']);
}
function MarkAsPending($id)
{
$sql = "UPDATE orders SET status='pending', date_shipped='' WHERE order_id=$id LIMIT 1";
mysql_query($sql);
}
function MarkAsShipped($id)
{
$today = date("Y-m-d");
$sql = "UPDATE orders SET status='shipped',date_shipped=$today WHERE order_id=$id LIMIT 1";
mysql_query($sql);
}
function MarkAsCancelled($id)
{
$sql = "UPDATE orders SET status='cancelled', date_shipped='' WHERE order_id=$id LIMIT 1";
mysql_query($sql);
}
function GetOrdersByStatus($status='shipped')
{
include_once('order_classes.php');
// this contains the OrderInfo class that does all the hard work gathering
// detailed information about each order.
// Stuff like who it was shipped to, which products and how many were included
// in the order,
// the total cost of the order etc.
$sql = "SELECT * FROM orders WHERE status='$status' ORDER BY date_ordered DESC";
$this->pgn8 = new pagination($sql, 5, "pgn8_$status_sql");
$pageResult = $this->pgn8->findPageResult();
while ($allorders = mysql_fetch_assoc($pageResult))
{
$o = new OrderInfo($allorders[order_id]);
$Orders[] = $o;
}
$this->Orders = $Orders;
}
}
OrderView.php
Code: Select all
<?
class OrderView
{
var $title = 'Shipped';
var $OrderModel;
function __construct($OrderModel)
{
$this->OrderModel = $OrderModel;
}
function setTitle($title)
{
$this->title= $title;
}
function getTitle()
{
return $this->title;
}
function ActionButtons($O)
{
echo '<ul>';
printf('<li><a href="%s?page=%s&cmd=markpending&orderID=%s">Mark as Pending</a></li>',thispage(),$_GET['page'],$O->Order['order_id'] );
echo '</ul>';
}
function Output()
{
echo $this->OrderModel->pgn8->showingFromTo();
echo $this->OrderModel->pgn8->displayPagination();
?>
<table class="datatable">
<thead>
<th scope="col">Order</th>
<th scope="col">Purchased Items</th>
<th scope="col">Status</th>
<th></th>
<th></th>
<thead>
<?
foreach($this->OrderModel->Orders as $O)
{
echo '<tr>';
echo '<td>';
echo '<b>'.date(" F d, Y", strtotime($O->Order[date_ordered])).'</b><br />';
echo 'Order ID: '.str_replace('-','',$O->Order[date_ordered]).'-'.$O->Order['order_id'].'<br />';
echo 'Bill to: '.$O->Billing[card_name]. '<br />';
echo 'Ship to: '.$O->Shipping[name];
echo '</td><td>';
foreach($O->Purchased as $P)
{echo $P[name].' x '.$P[quantity].'<br />'; }
echo '<b>Subtotal: $'.number_format($O->Order[subtotal],2).'</b>';
echo '</td><td>';
echo ucwords($O->Order[status]);
echo '</td><td>';
$this->ActionButtons($O);
echo '</td><td>';
printf('<a class="view" href="order_view.php?page=%s&orderID=%s">View Details</a>',$page,$O->Order['order_id'] );
echo '</td></tr>';
echo '<tr><td colspan="5" class="xhrdetail"></td></tr>';
}
echo '</table>';
echo $this->OrderModel->pgn8->displayPagination();
}
} // end Orderview class
class OrderView_Pending extends OrderView
{
var $title = "Pending";
function ActionButtons($O)
{
echo '<ul>';
printf('<li><a href="%s?page=%s&cmd=markshipped&orderID=%s">Mark as Shipped</a></li>',thispage(),$_GET['page'],$O->Order['order_id'] );
printf('<li><a href="%s?page=%s&cmd=markcancelled&orderID=%s">Cancel Order</a></li>',thispage(),$_GET['page'],$O->Order['order_id'] );
echo '</ul>';
}
}
?>
OrderTemplate.php
Code: Select all
<?
class OrderTemplate
{
function __construct($ViewOBJ)
{
$this->ViewOBJ = $ViewOBJ;
//var_dump($this->ViewOBJ->OrderModel);
}
function Render()
{
global $ADMINHEAD;
global $CONFIG;
$H = new HTMLHead($ADMINHEAD);
$H->PrefixTitle("Shipped Orders - ");
$H->LinkCSS('orders.css');
$H->ExtraCSS('#leftbar li#orders-sub, #leftbar li#orders-sub .sub-menu{display:block;}');
echo $H->Output();
?>
<div id="admin-container">
<?
include('../includes/inc.admin_header.php');
include('../includes/inc.admin_navigation.php');
?>
<div id="content">
<h2><?php echo $this->ViewOBJ->getTitle(); ?> Orders</h2>
<?php echo $this->ViewOBJ->Output(); ?>
</div>
<? include('../includes/inc.admin_footer.php'); ?>
</div>
</body>
</html>
<?
}
} //end class
?>
Posted: Fri Sep 14, 2007 4:20 pm
by Christopher
Definitely getting there. You fixed my typos and oversights.
Making $Orders public was the right thing to do (others may argue though).
I can see why you did what you did with $pgn8, but I think a better long term solution would be to split your paginator in two -- so the Model would just supply the database adapter interface to the HTML generation part created in the View. The you could create the paginator in the View and pass the Model to it. That would make things cleaner.
Posted: Fri Sep 14, 2007 8:20 pm
by thinsoldier
I got frustrated and made everything public

I was getting a lot of errors regarding methods of non-objects and attempting to access private stuff so I just made everything public. I'll go back and make things private 1 by 1 to see what needs to be public and what doesn't.
I can see why you did what you did with $pgn8, but I think a better long term solution would be to split your paginator in two -- so the Model would just supply the database adapter interface to the HTML generation part created in the View. The you could create the paginator in the View and pass the Model to it. That would make things cleaner.
I don't know why but I have no idea what that means at the moment
if you could use 3 words other than Model, View, Controller, what would they be?
Is M-V-C really so much better than M-VC in every instance?
Posted: Fri Sep 14, 2007 11:58 pm
by Christopher
thinsoldier wrote:Is M-V-C really so much better than M-VC in every instance?
There is no MVC in this at all, just simple layer separation between the presentation code and the domain code (M-VC). I would recommend getting comfortable with Three Tier design first where you clearly separate the presentation, domain model and data access layers. Lower layers should have no dependencies on higher layers. That will lay the foundation for more reuse and code that is easier to maintain.
MVC is a whole other discussion, but MVC does not make much sense until you really understand things like N-Tier architecture. As frustrating as is may seem I think you will find the code more flexible as you implement more different pages on top of it.