Home » Forums » Ubercart » Ideas and Suggestions

Optimizing uc_order_search_form_submit

Submitted by pebosi@drupal.org on Fri, 03/28/2008 - 14:32
pebosi@drupal.org
Posts: 20
 Joined: 09/05/2007
Uber DonorBug Finder

Hi, i found the function uc_order_search_form_submit and saw this:

  if (strlen(trim($form_values['billing_first_name'])) == 0) {
    $billing_first_name = '0';
  }
  else {
    $billing_first_name = strtolower(trim($form_values['billing_first_name']));
  }

  if (strlen(trim($form_values['billing_last_name'])) == 0) {
    $billing_last_name = '0';
  }
  else {
    $billing_last_name = strtolower(trim($form_values['billing_last_name']));
  }

  if (strlen(trim($form_values['billing_company'])) == 0) {
    $billing_company = '0';
  }
  else {
    $billing_company = strtolower(trim($form_values['billing_company']));
  }

  if (strlen(trim($form_values['shipping_first_name'])) == 0) {
    $shipping_first_name = '0';
  }
  else {
    $shipping_first_name = strtolower(trim($form_values['shipping_first_name']));
  }

  if (strlen(trim($form_values['shipping_last_name'])) == 0) {
    $shipping_last_name = '0';Hunsrück Immobilien
  }
  else {
    $shipping_last_name = strtolower(trim($form_values['shipping_last_name']));
  }

  if (strlen(trim($form_values['shipping_company'])) == 0) {
    $shipping_company = '0';
  }
  else {
    $shipping_company = strtolower(trim($form_values['shipping_company']));
  }

i thought it would be better to change it like this:

  $search_fields = array('billing_first_name', 'billing_last_name', 'billing_company', 'shipping_first_name', 'shipping_last_name', 'shipping_company');
 
  foreach($search_fields as $search_field) {
    if (empty($form_values[$search_field])) {
      ${$search_field} = '0';
    } else {
      ${$search_field} = strtolower(trim($form_values[$search_field]));
    }
  }

or did i miss something?

regards pebosi
permalink #1 Fri, 03/28/2008 - 14:44
Ryan

Posts: 5379
 Joined: 08/07/2007
AdministratorHead Code Monkey - I eat bugs.

Good call. To avoid the difficult to read interpreted variable names, I think I'll go a step further than your code here and just store the value in a second array. Then I'll implode this array to generate the path instead of concatenating all the individual variables together.

permalink #2 Fri, 03/28/2008 - 14:46
pebosi@drupal.org
Posts: 20
 Joined: 09/05/2007
Uber DonorBug Finder

Ryan wrote:
Then I'll implode this array to generate the path instead of concatenating all the individual variables together.

Yeah Eye-wink That's my second suggestion for this function. Great!

regards pebosi

permalink #3 Fri, 03/28/2008 - 14:51
Ryan

Posts: 5379
 Joined: 08/07/2007
AdministratorHead Code Monkey - I eat bugs.

Now it all looks like:

<?php
function uc_order_search_form_submit($form_id, $form_values) {
  $keys = array(
    'billing_first_name',
    'billing_last_name',
    'billing_company',
    'shipping_first_name',
    'shipping_last_name',
    'shipping_company',
  );

  foreach ($keys as $key) {
    if (strlen(trim($form_values[$key])) == 0) {
      $args[] = '0';
    }
    else {
      $args[] = strtolower(trim($form_values[$key]));
    }
  }

  if ($form_values['use_dates']) {
    $args[] = mktime(0, 0, 0, $form_values['start_date']['month'], $form_values['start_date']['day'], $form_values['start_date']['year']);
    $args[] = mktime(23, 59, 59, $form_values['end_date']['month'], $form_values['end_date']['day'], $form_values['end_date']['year']);
  }
  else {
    $args[] = '0';
    $args[] = '0';
  }

  drupal_goto('admin/store/orders/search/results/'. implode('/', $args));
}
?>

Very tidy. Smiling

permalink #4 Fri, 03/28/2008 - 15:11
pebosi@drupal.org
Posts: 20
 Joined: 09/05/2007
Uber DonorBug Finder

Real nice Smiling

Can you tell me why to use

    if (strlen(trim($form_values[$key])) == 0) {

and not just

    if (empty($form_values[$key])) {

?

regards pebosi

permalink #5 Fri, 03/28/2008 - 15:17
Ryan

Posts: 5379
 Joined: 08/07/2007
AdministratorHead Code Monkey - I eat bugs.

heh Because I didn't see you'd made that change. I'll go ahead and do it but keep the trim() in there. The main reason for the trim is so we won't bother searching for a space or a line full of spaces, especially if it was accidentally entered.

permalink #6 Fri, 03/28/2008 - 15:26
pebosi@drupal.org
Posts: 20
 Joined: 09/05/2007
Uber DonorBug Finder

ok, thanks.

permalink #7 Fri, 03/28/2008 - 19:14
pebosi@drupal.org
Posts: 20
 Joined: 09/05/2007
Uber DonorBug Finder

Just tested the newest bzr version but theres an error you have to

replace
if (empty(trim($form_values[$key]))) {

with
if (trim(empty($form_values[$key]))) {

otherwise there is the following error:

Fatal error: Can't use function return value in write context

regards pebosi

Edit:
Sorry, i think the following would be better:

  foreach ($keys as $key) {
    $form_values[$key] = trim($form_values[$key]);
    if (empty($form_values[$key])) {
      $args[] = '0';
    }
    else {
      $args[] = strtolower($form_values[$key]);
    }
  }

permalink #8 Fri, 03/28/2008 - 22:53
Ryan

Posts: 5379
 Joined: 08/07/2007
AdministratorHead Code Monkey - I eat bugs.

hehe Yeah, now I remember why I was using strlen() instead of empty(). Since it seems like the most concise way to write the condition, I've just reverted to the previous code.