Help with code optimization

[fRAiLtY-]

Hi guys,

 

I've managed to cobble together a PHP script which works perfectly, there are no faults or errors and it does exactly as intended. However I asked for some code critique and was advised that I could chop it up into smaller methods rather than one monster method to make it better or more efficient, as it's been added to quite regularly over the last few weeks, now it's finished I want to tidy it up.

 

Can anyone provide some amendments or advice on how to make it a bit more streamline or tidy by perhaps creating additional methods with what I have or similar.

 

Many thanks

 

<?php
class Tharstern_TharCustomer_Model_Observer {
protected function despatchDate($startdate, $businessdays, $dateformat = 'c') {
	//vars
	$i = 1;
	$holidays = array("02-01-2012", "06-04-2012", "09-04-2012", "07-05-2012", "04-06-2012", "05-06-2012", "28-06-2012", "27-08-2012", "25-12-2012", "26-12-2012");
	//start date depends on time >11pm +1day
	if(date('H') >= 11) {
		$dayx = strtotime($startdate. '+1 day');
	} else $dayx = strtotime($startdate);
	$businessdays = ceil($businessdays);
	//loop over get preliminary date
	while($i < $businessdays) {
		$day = date('N',$dayx);
		$date = date('d-m-Y',$dayx);
		if($day < 6 && !in_array($date,$holidays))
						$i++;
		$dayx = strtotime($date.' +1 day');
	}
	//if preliminary date is weekend/bank holiday iterate over until neither
	$day = date('N',$dayx);
	$date = date('d-m-Y',$dayx);
	while($day >= 6 || in_array($date,$holidays)) {
		$dayx = strtotime($date.' +1 day');
		$day = date('N',$dayx);
		$date = date('d-m-Y',$dayx);
	}
	//return final date for despatch
	return date($dateformat, $dayx);
}
public function checkCustomer($observer) {
	// connect to Tharstern
	$cnx = mssql_connect('xxxxxxx');
	mssql_select_db('TharData');
	// get customers billing name and email address   
	$session = Mage::getSingleton('customer/session');
	$customer = $session->getCustomer();
	// get details
	$order = $observer->getEvent()->getOrder();
	$orderId = $observer->getEvent()->getOrder()->getIncrementId();
	$invAdd = $observer->getEvent()->getOrder()->getBillingAddress();
	$productsCollection = $observer->getEvent()->getOrder()->getItemsCollection();
	foreach($productsCollection as $_products) {
		if ($allOptions = $_products->getData('product_options')) {
			$options = unserialize($allOptions);
			if (isset($options['options'])) {
				$optionDays = 0;
				foreach ($options['options'] as $optionValues) {
					if ($optionValues['value']) {
						$_printValue = isset($optionValues['print_value']) ? $optionValues['print_value'] : strip_tags($optionValues['value']);
						$values = explode(', ', $_printValue);
						foreach ($values as $value) {
							preg_match('/(\d+)/', $value, $option);
							if(!isset($option[0]) || $option[0] == 24) {
								$extraDays = 0;
							} else $extraDays = $option[0];
							$optionDays += $extraDays;
						}
					}
				}
			}
		}
	}
	$items = $order->getAllVisibleItems();
	foreach ($items as $item) {
		$product = Mage::getModel('catalog/product')->load($item->getProductId());
		$turnaround = $product->getAttributeText('turnaround');
		preg_match('/(\d+)/', $turnaround, $base);
		if(!isset($base[0]) || $base[0] == 24) {
			$baseDays = 0;
		} else $baseDays = $base[0];
		$finalDespatchDate = $this->despatchDate(date('d-m-Y'), $optionDays + $baseDays);
	}
	// check if a company is specified otherwise use the billing name
	$identifier = $customer->getName();
	//if(isset($customer->getCompany())){
	//$identifier = $customer->getCompany();
	//}
	// query database and check for customers existence
	$check = mssql_query("SELECT Customers.Code, Customers.Name, Customers.Address, Customers.Town, Customers.County, Customers.Postcode, CustomerContacts.Email, Customers.Ref3
							FROM Customers
							INNER JOIN CustomerContacts
							ON Customers.Name=CustomerContacts.Name
							WHERE Customers.Name ='".$identifier."' AND CustomerContacts.Email ='".$customer->getEmail()."' AND Ref3 = 'Advantage' AND Address = '".$invAdd->getStreet(1)."' AND Postcode = '".$invAdd->getPostcode()."'
							GROUP BY Customers.Code, Customers.Name, Customers.Address, Customers.Town, Customers.County, Customers.Postcode, CustomerContacts.Email, Customers.Ref3");
	// check results
	if(mssql_num_rows($check) == 1) {
		// echo customer code
		$row = mssql_fetch_array($check);
		$tharCode = $row['Code'];
		// Mage_Core db connection to update order column if applicable
		$write = Mage::getSingleton('core/resource')->getConnection('core_write');
		$write->query("UPDATE sales_order SET customer_tharstern_code = '".$tharCode."', final_despatch_date = '".$finalDespatchDate."' WHERE increment_id = '".$orderId."' LIMIT 1");
		if(!$check) {
			error_log("Error checking Tharstern for customer",1,"xxxxxxx");
		}
	} elseif(mssql_num_rows($check) == 0) {
		// get initials and uppercase them if necessary
		$initials = '';
		foreach(explode(' ', $customer->getName()) as $word) {
			$initials .= strtoupper($word[0]);
		}
		// set integer and string values to bendart 6-digit convention
		$codeNum = 0;
		$codeStr = $initials.'0001';
		// iterate through results until no match is found
		do {
			$codeNum++;
			$codeStr = $initials . str_pad($codeNum, 4, '0', STR_PAD_LEFT);
			$query = mssql_query("SELECT * FROM Customers WHERE Code = '".$codeStr."'");
		}
		while(mssql_num_rows($query) == 1);
		// Mage_Core db connection to update order column with new customers code
		$write = Mage::getSingleton('core/resource')->getConnection('core_write');
		$write->query("UPDATE sales_order SET customer_tharstern_code = '".$tharCode."', final_despatch_date = '".$finalDespatchDate."' WHERE increment_id = '".$orderId."' LIMIT 1");
		// add customer to Tharstern for processing
		$addCustomer = mssql_query("INSERT INTO Customers(Code,Name,Address,Town,County,Postcode,Ref1,Ref2,Ref3,PostingAccount)
									VALUES ('".$codeStr."','".$customer->getName()."','".$invAdd->getStreet(1)."','".$invAdd->getCity()."','".$invAdd->getRegion()."','".$invAdd->getPostcode()."','Web Sales','Retail','Advantage','CASHCU');
									INSERT INTO CustomerContacts(ParentCustomer,Name,Email,EmailName,PrimaryContact,GMDisabled)
									VALUES ('".$codeStr."','".$customer->getName()."','".$customer->getEmail()."','".$customer->getName()." (".$customer->getEmail().")','1','1')");
		// check result and e-mail
		if(!$addCustomer) {
			error_log("Error adding customer to Tharstern",1,"tom@bendart.co.uk");
		} else {
			// write new customer details to csv for logging purposes.
			$csv = $_SERVER['DOCUMENT_ROOT']."/var/log/newcustomersadded.csv";
			$handle = fopen($csv, 'a');
			$data = "\n".date ('Y-m-d H:i:s').",".$orderId.",".$codeStr.",".$customer->getName();
			fwrite($handle, $data);
			fclose($handle);
			mail('xxxx,'New Customer', 'New Customer Created: '.$codeStr);
		}
	}
}
}

[Christian F.]

I've taken the liberty to un-nest your code a bit, to make it easier to read:

<?php

class Tharstern_TharCustomer_Model_Observer {

protected function despatchDate ($startdate, $businessdays, $dateformat = 'c') {
	//vars
	$i = 1;
	$holidays = array ("02-01-2012", "06-04-2012", "09-04-2012", "07-05-2012", "04-06-2012",
						"05-06-2012", "28-06-2012", "27-08-2012", "25-12-2012", "26-12-2012");

	//start date depends on time >11pm +1day
	if (date ('H') >= 11) {
		$dayx = strtotime ($startdate . '+1 day');
	} else {
		$dayx = strtotime ($startdate);
	}

	$businessdays = ceil ($businessdays);

	//loop over get preliminary date
	while ($i < $businessdays) {
		$day = date ('N', $dayx);
		$date = date ('d-m-Y', $dayx);

		if ($day < 6 && !in_array ($date, $holidays)) {
			$i++;
		}

		$dayx = strtotime ($date . ' +1 day');
	}

	//if preliminary date is weekend/bank holiday iterate over until neither
	$day = date ('N', $dayx);
	$date = date ('d-m-Y', $dayx);

	while ($day >= 6 || in_array ($date, $holidays)) {
		$dayx = strtotime ($date . ' +1 day');
		$day = date ('N', $dayx);
		$date = date ('d-m-Y', $dayx);
	}

	//return final date for despatch
	return date ($dateformat, $dayx);
}

public function checkCustomer ($observer) {
	// connect to Tharstern
	$cnx = mssql_connect ('xxxxxxx');
	mssql_select_db ('TharData');

	// get customers billing name and email address   
	$session = Mage::getSingleton ('customer/session');
	$customer = $session->getCustomer ();

	// get details
	$order = $observer->getEvent ()->getOrder ();
	$orderId = $observer->getEvent ()->getOrder ()->getIncrementId ();
	$invAdd = $observer->getEvent ()->getOrder ()->getBillingAddress ();
	$productsCollection = $observer->getEvent ()->getOrder ()->getItemsCollection ();

	foreach ($productsCollection as $_products) {
		if (!$allOptions = $_products->getData ('product_options')) {
			continue;
		}

		$options = unserialize ($allOptions);

		if (!isset ($options['options'])) {
			continue;
		}

		$optionDays = 0;

		foreach ($options['options'] as $optionValues) {
			if (!$optionValues['value']) {
				continue;
			}

			$_printValue = isset ($optionValues['print_value']) ? $optionValues['print_value'] : strip_tags ($optionValues['value']);
			$values = explode (', ', $_printValue);

			foreach ($values as $value) {
				preg_match ('/(\d+)/', $value, $option);

				if (!isset ($option[0]) || $option[0] == 24) {
					$extraDays = 0;
				} else {
					$extraDays = $option[0];
				}

				$optionDays += $extraDays;
			}
		}
	}

	$items = $order->getAllVisibleItems ();

	foreach ($items as $item) {
		$product = Mage::getModel ('catalog/product')->load ($item->getProductId ());
		$turnaround = $product->getAttributeText ('turnaround');
		preg_match ('/(\d+)/', $turnaround, $base);

		if (!isset ($base[0]) || $base[0] == 24) {
			$baseDays = 0;
		} else {
			$baseDays = $base[0];
		}

		$finalDespatchDate = $this->despatchDate (date ('d-m-Y'), $optionDays + $baseDays);
	}

	// check if a company is specified otherwise use the billing name
	$identifier = $customer->getName ();

//		if(isset($customer->getCompany())){
//			$identifier = $customer->getCompany();
//		}

	// query database and check for customers existence
	$check = mssql_query ("SELECT Customers.Code, Customers.Name, Customers.Address, Customers.Town, Customers.County, Customers.Postcode, CustomerContacts.Email, Customers.Ref3
							FROM Customers
							INNER JOIN CustomerContacts
							ON Customers.Name=CustomerContacts.Name
							WHERE Customers.Name ='" . $identifier . "' AND CustomerContacts.Email ='" . $customer->getEmail () . "' AND Ref3 = 'Advantage' AND Address = '" . $invAdd->getStreet (1) . "' AND Postcode = '" . $invAdd->getPostcode () . "'
							GROUP BY Customers.Code, Customers.Name, Customers.Address, Customers.Town, Customers.County, Customers.Postcode, CustomerContacts.Email, Customers.Ref3");

	// check results
	if (mssql_num_rows ($check) == 1) {
		// echo customer code
		$row = mssql_fetch_array ($check);
		$tharCode = $row['Code'];

		// Mage_Core db connection to update order column if applicable
		$write = Mage::getSingleton ('core/resource')->getConnection ('core_write');
		$write->query ("UPDATE sales_order SET customer_tharstern_code = '" . $tharCode . "', final_despatch_date = '" . $finalDespatchDate . "' WHERE increment_id = '" . $orderId . "' LIMIT 1");

		if (!$check) {
			error_log ("Error checking Tharstern for customer", 1, "xxxxxxx");
		}

		return;
	} 

	if (mssql_num_rows ($check) != 0) {
		return;
	}

	// get initials and uppercase them if necessary
	$initials = '';
	foreach (explode (' ', $customer->getName ()) as $word) {
		$initials .= strtoupper ($word[0]);
	}

	// set integer and string values to bendart 6-digit convention
	$codeNum = 0;
	$codeStr = $initials . '0001';

	// iterate through results until no match is found
	do {
		$codeNum++;
		$codeStr = $initials . str_pad ($codeNum, 4, '0', STR_PAD_LEFT);
		$query = mssql_query ("SELECT * FROM Customers WHERE Code = '" . $codeStr . "'");
	} while (mssql_num_rows ($query) == 1);

	// Mage_Core db connection to update order column with new customers code
	$write = Mage::getSingleton ('core/resource')->getConnection ('core_write');
	$write->query ("UPDATE sales_order SET customer_tharstern_code = '" . $tharCode . "', final_despatch_date = '" . $finalDespatchDate . "' WHERE increment_id = '" . $orderId . "' LIMIT 1");

	// add customer to Tharstern for processing
	$addCustomer = mssql_query ("INSERT INTO Customers(Code,Name,Address,Town,County,Postcode,Ref1,Ref2,Ref3,PostingAccount)
								VALUES ('" . $codeStr . "','" . $customer->getName () . "','" . $invAdd->getStreet (1) . "','" . $invAdd->getCity () . "','" . $invAdd->getRegion () . "','" . $invAdd->getPostcode () . "','Web Sales','Retail','Advantage','CASHCU');
								INSERT INTO CustomerContacts(ParentCustomer,Name,Email,EmailName,PrimaryContact,GMDisabled)
								VALUES ('" . $codeStr . "','" . $customer->getName () . "','" . $customer->getEmail () . "','" . $customer->getName () . " (" . $customer->getEmail () . ")','1','1')");

	// check result and e-mail
	if (!$addCustomer) {
		error_log ("Error adding customer to Tharstern", 1, "tom@bendart.co.uk");
		return;
	}

	// write new customer details to csv for logging purposes.
	$csv = $_SERVER['DOCUMENT_ROOT'] . "/var/log/newcustomersadded.csv";
	$handle = fopen ($csv, 'a');
	$data = "\n" . date ('Y-m-d H:i:s') . "," . $orderId . "," . $codeStr . "," . $customer->getName ();
	fwrite ($handle, $data);
	fclose ($handle);
	mail (xxxx, "New Customer", "New Customer Created: " . $codeStr);
}
}

 

As for critique on the code itself... I can't give you very detailed criticism I'm afraid, seeing as I don't know exactly what the code is meant to do. I can see what it tries to do, but without having some source data to play with I can't be 100% sure it does things in a good/optimal manner.

 

However, there are some thing that I spot right away, as being suspect if nothing else. First up is that 3 levels of foreach loop you have, with a preg_match () in the innermost. This is something I suspect can be written a whole lot cleaner, with perhaps only one level of nesting in the loops. Generally speaking nesting loops is something you really don't want to do, as it racks up system resource usage like nothing else.

For instance, it seems to be that you could have done away with the explode () and innermost loop, and used preg_match_all () instead.

 

The getting of all details could be refactored into its own method as well, where it returns the $optionDays (and possibly other required data).

Same goes for the bit where you retrieve items, adding the customer to "Tharsten", sending e-mail and the iteration bit just above that.

 

Speaking of which: What, exactly, are you doing with the "initials" iteration, and why all of those SQL queries? If there's one thing that's worse than nesting loops, then it's nesting SQL queries inside loops. I highly recommend finding another way to write this, so that you only need one query per user.

Depending upon your reason for doing things like that in the first place, you might want to rethink the whole logic behind those "initials" as well. This seems like something that an auto_increment in the database is made for.

[gizmola]

The structure of the class is really something you should decide upon, and that requires context.  How often will this be built upon?  Will you be adding similar features to the system that could reuse some of the class in whole or part?  Have you applied DRY? Are there clear patterns of repetition that could be refactored into small methods?  Would you like to build unit tests for your code, so that as things are added to or changed in the future, you could catch breakage in the future? 

 

It looks like you're implementing some design patterns already, so as long as the classes involved implement your intentions I wouldn't be concerned.

 

With that said there some things that raise some reuse flags.  For example, you have a hardwired holidays list: 

 

$holidays = array("02-01-2012", "06-04-2012", "09-04-2012", "07-05-2012", "04-06-2012", "05-06-2012", "28-06-2012", "27-08-2012", "25-12-2012", "26-12-2012");  

 

Is this system going to expire at the end of the year?  If not, I would think you would want to be able to update that list in some way, without having to change the code of this class.

 

Later you do logging and mailing of information.  I would again assume that logging to a file is something you may want to do in other places, and a log class would be advantageous.

 

$csv = $_SERVER['DOCUMENT_ROOT']."/var/log/newcustomersadded.csv";
			$handle = fopen($csv, 'a');
			$data = "\n".date ('Y-m-d H:i:s').",".$orderId.",".$codeStr.",".$customer->getName();
			fwrite($handle, $data);
			fclose($handle);
			mail('xxxx,'New Customer', 'New Customer Created: '.$codeStr);

 

All the database code is very specific which makes me question the purpose of this class, considering it appears to be designed to process certain types of events.  It's not clear how that would be put together in your final application. You also have a lot of DRY database code.  A wrapper class or ORM would be helpful in that regard, but even more so, I can't recommend enough the use of PDO.

[Christian F.]

Oh, yeah. Almost forgot:

Instead of the fopen (); fwrite (); fclose (); dance, you really should be using file_put_contents () instead.

[Psycho]

Well, I would start by looking at all that nested functionality. I try to avoid that as it gets hard to understand where each if/while/etc section of code is ending. Also, if you have a large block of code executing with a loop or if, you can always break that out into a separate method. It looks like that, aside from the 'despatchDate' method you simply created a long procedural block of code in the 'checkCustomer' method.

 

It also looks like you may be storing serialized arrays in the database. That is typically a bad practice.

 

And here is something that caught my eye. Look at these two small chunks of code

 

if(!isset($option[0]) || $option[0] == 24)
{
    $extraDays = 0;
}
else $extraDays = $option[0];
$optionDays += $extraDays;

 

if(!isset($base[0]) || $base[0] == 24)
{
    $baseDays = 0;
}
else $baseDays = $base[0];

 

These are essentially doing the exact same thing. You could just create a method to do that logic.

private function addDays($days = 0)
{
    return ($days!=24) ? $days : 0 ;
}

 

Then where you had those two blocks of code previously you could just do this:

 

$optionDays += $this->addDays($option[0]);

 

$baseDays = $this->addDays($base[0]);

 

That's just an off the cuff suggestion, but I'd need to better understand the intent to really provide a 'valid' solution.

[fRAiLtY-]

Hi all,

 

Thanks for the responses, I should have provided more context.

 

Ultimately, it's an imperfect solution to an unavoidable problem. Our Linux web server running Magento has to link to our in-house MIS server running MSSQL. There is nothing there that can change, hence the mssql_* usage.

 

Initially the checkCustomer method was very small, now it's grown significantly. It hooks into standard Magento event observers and is triggered when the "Place Order" is pressed, it works in the following way:

 

1) Gets details about the current order, and the customer involved.

2) Connects to the external MSSQL database, uses the name and first line of the address to check for existance in the database. If a match is found, then fine.

3) If not, it takes the first letters of both the first and surname, adds 0's to make it up to 6 characters long and iterates over until no match is found in the database, then adds the customer with those details. Our customer naming convention is this way, i.e Joe Bloggs becomes JB00001 unless that exists, then increment up until it doesn't. If it adds a customer I log it and e-mail myself.

4) It then iterates over the custom options selected on the product, looks for additional days (Our options follow this convention: Selected option +1day) so it plucks the integer out for each instance of the option, adds them up, then adds them to a pre-determined number, then adds that to the days function ending up with a lead-time.

 

As I write it, it sounds confusing, but it's actually quite simple!

 

Cheers.

[Christian F.]

3) If not, it takes the first letters of both the first and surname, adds 0's to make it up to 6 characters long and iterates over until no match is found in the database, then adds the customer with those details. Our customer naming convention is this way, i.e Joe Bloggs becomes JB00001 unless that exists, then increment up until it doesn't. If it adds a customer I log it and e-mail myself.

This can be done quite a bit better, by having the MySQL query return all possible matches (or even the "max value). Then use PHP's string manipulation capabilities to increase the value. All you need is substr (), intval () and sprintf (), coupled with the normal operators.

 

Step number 4 sounds like it could be unrolled as well, and made a whole lot simpler. PHP does have some very good methods for sorting and extracting information from string, arrays and objects. Using them whenever possible, instead of rolling your own solutions, is highly recommended.

[gizmola]

Ultimately, it's an imperfect solution to an unavoidable problem. Our Linux web server running Magento has to link to our in-house MIS server running MSSQL. There is nothing there that can change, hence the mssql_* usage.

 

Here's where I question whether or not what I wrote previously was too cryptic.  Yes you most certainly can change something.  You can use PDO.  See http://www.php.net/manual/en/ref.pdo-sqlsrv.php.  Not unlike ODBC, PDO will give you a single interface that lets you talk to both the databases, and it has bind variables, the use of which eliminates the need to worry about sql injection, or the need to run escape routines on strings.

[fRAiLtY-]

From what I can see, the only driver that works with PHP on Linux is this: http://www.microsoft.com/en-us/download/details.aspx?id=28160

 

Does that seem right? Then hopefully I can take advantage of PDO to better prepare the queries.

 

I'll look at some of the other suggestions too.

 

Cheers!

[gizmola]

From what I can see, the only driver that works with PHP on Linux is this: http://www.microsoft.com/en-us/download/details.aspx?id=28160

 

Does that seem right? Then hopefully I can take advantage of PDO to better prepare the queries.

 

I'll look at some of the other suggestions too.

 

Cheers!

 

What client library do you currently have installed?  It ought to work with that one already....

[fRAiLtY-]

Currently I just pdo_mysql and the standard MSSQL modules installed. I can connect using mssql_* functions but it doesn't handle prepared statements I think.

 

I tried PDO_DBLIB but this hasn't been developed since 2005 and doesn't work with my versions of software due to it's age.

[kicken]

This SQL query could replace your loop to find the next available customer number. It will select the highest existing customer number for the given initials and add one to it. If nobody with the given initials has been added yet it will return 1. This small bit could be made into a method.

SELECT COALESCE(MAX(CONVERT(INT, SUBSTRING(Code, 3, LEN(Code)))), 1) FROM Customers WHERE Code LIKE 'JB%'

 

private function getNextCustomerNumber($initials){
   $res=mssql_query("SELECT COALESCE(MAX(CONVERT(INT, SUBSTRING(Code, 3, LEN(Code)))), 1) FROM Customers WHERE Code LIKE '{$initials}%'");
   $row=mssql_fetch_array($res, MSSQL_NUM);
   return sprintf("%s%04d", $initials, $row[0]);
}

 

That would accept the initials as input (ie, 'JB') and return as output your customer # string, (ie 'JB0025'). The only problem with the query is it assumes two-letter initials. Your for loop to generate $initials is written in such a way that is it may be possible to have more than two letters as the initials. You could either modify the query to allow for additional letters, or modify the loop to limit it to two letters.

Edited September 24, 2012 by kicken