Home » Download » Modules » Flexifilter » Issues

Optimize the code

kiamlaluno - November 12, 2008 - 13:31
Project:Flexifilter
Version:6.x-1.1-rc1
Component:Code - Misc
Category:task
Priority:normal
Assigned:Unassigned
Status:active
Description

The code needs to be optimized.
Code like

<?php
  if ($value == TRUE) {
?>

can be rewritten like

<?php
  if ($value) {
?>

This is just an example, but the code needs a general optimization.

» Login or register to post comments

#1

kiamlaluno - November 12, 2008 - 14:00

Another example is _flexifilter_push_conf() (flexifilter.components.inc), where the foreach-loop can be replaced with a call to array_merge().

<?php
function _flexifilter_push_conf($settings) {
  global $conf;
  $conf_backup = $conf;
  foreach ($settings as $key => $value) {
    $conf[$key] = $value;
  }
  return $conf_backup;
}
?>

<?php
function _flexifilter_push_conf($settings) {
  global $conf;

  $conf_backup = $conf;
  $conf = array_merge($conf, $settings);

  return $conf_backup;
}
?>

Login or register to post comments

#2

kiamlaluno - November 13, 2008 - 20:04

Also in flexifilter_get_component_list() the following code is used:

<?php
    foreach ($components as $key => $component) {
      if (!isset($component['group'])) {
        $components[$key]['group'] = t('Other');
      }
      if (!isset($component['description'])) {
        $components[$key]['description'] = '';
      }
      if (!isset($component['callback_arguments'])) {
        $components[$key]['callback_arguments'] = array();
      }
      if (!isset($component['contains_condition'])) {
        $components[$key]['contains_condition'] = FALSE;
      }
      if (!isset($component['contains_components'])) {
        $components[$key]['contains_components'] = FALSE;
      }
      if (!isset($component['is_container'])) {
        $components[$key]['is_container'] = $components[$key]['contains_condition'] || $components[$key]['contains_components'];
      }
      if (!isset($component['is_advanced'])) {
        $components[$key]['is_advanced'] = FALSE;
      }
      if (!$include_advanced_items && $components[$key]['is_advanced']) {
        unset($components[$key]);
      }
    }
?>

Generally speaking, if I want to be sure an array contains some values I use the following code:

<?php
  $default_values = array(
    // Put here the needed default values.
  );
  $values = array_merge($default_values, $values);
?>

In this way, the keys which aren't $values but are in $default_values will take the default values, without the need to check every keys of the array.

In the specific, the foreach loop would become:

<?php
    $default_values = array(
      'group' => t('Other'),
      'description' => '',
      'callback_arguments' => array(),
      'contains_condition' => FALSE,
      'contains_components' => FALSE,
      'is_advanced' => FALSE,
    );
   
    foreach ($components as $key => $component) {
      $components[$key] = array_merge($default_values, $component[$keys]);
      $components[$key]['is_container'] = $components[$key]['contains_condition'] || $components[$key]['contains_components'];
      if (!$include_advanced_items && $components[$key]['is_advanced']) {
        unset($components[$key]);
      }
    }
?>

Login or register to post comments

#3

kiamlaluno - November 13, 2008 - 17:13

In flexifilter_get_filter_by_delta() (flexifilter.module), the following code

<?php
  static $cache;
  if (!isset($cache)) {
    $cache = array();
  }
?>

can be simplified in

<?php
  static $cache = array();
?>

Login or register to post comments

#4

Steven Jones - November 13, 2008 - 17:17

Good points, but, provide a patch please.

Login or register to post comments

#5

kiamlaluno - November 13, 2008 - 18:29

I am not sure if that could be helpful, as the maintainer is going to update the module with a 6.x-1.1 version.
For now, these are just suggestions; when I see the new release, I will eventually make a patch.

Login or register to post comments

#6

kiamlaluno - November 13, 2008 - 19:31

In _flexifilter_filter_from_db_row() there is the following lines of code

<?php
    'enabled' => ($row->enabled == 1),
    'advanced' => ($row->advanced == 1),
?>

which can be rewritten like

<?php
    'enabled' => $row->enabled,
    'advanced' => $row->advanced,
?>

as PHP, in every contexts where a boolean value is expected, consider any value different from 0 like TRUE.

Therefore, the following code

<?php
  $filter = array(
    'label' => $row->label,
    'description' => $row->description,
    'id' => $row->fid,
    'enabled' => ($row->enabled == 1),
    'advanced' => ($row->advanced == 1),
    'delta' => $row->delta,
  );
?>

could also be rewritten like

<?php
  $filter = $row;
?>

if $row doesn't contain any values apart the ones showed there.

Login or register to post comments

#7

kiamlaluno - November 13, 2008 - 19:41

flexifilter_get_number_enabled_filters() use the following code:

<?php
function flexifilter_get_number_enabled_filters($reset = FALSE) {
  static $count;
  if (!isset($count) || $reset) {
    $count = 0;
    $filters = flexifilter_get_filters($reset);
    foreach ($filters as $filter) {
      if ($filter['enabled']) {
        $count++;
      }
    }
  }
  return $count;
}
?>

It can be changed in:

<?php
function flexifilter_get_number_enabled_filters($reset = FALSE) {
  static $count = 0;
  if ($reset) {
    $count = 0;
    $filters = flexifilter_get_filters($reset);
    foreach ($filters as $filter) {
      if ($filter['enabled']) {
        $count++;
      }
    }
  }
  return $count;
}
?>

Login or register to post comments

#8

kiamlaluno - November 13, 2008 - 19:57

The code of flexifilter_get_unused_delta() is the following:

<?php
function flexifilter_get_unused_delta() {
  $deltas = array();
  for ($i = 0; $i < FLEXIFILTER_MAX_FILTERS; $i++) {
    $deltas[$i] = TRUE;
  }
  foreach (flexifilter_get_filters(FALSE) as $flexifilter) {
    if ($flexifilter['enabled']) {
      $deltas[$flexifilter['delta']] = FALSE;
    }
  }
  for ($i = 0; $i < FLEXIFILTER_MAX_FILTERS; $i++) {
    if ($deltas[$i] == TRUE) {
      return $i;
    }
  }
  return FALSE;
}
?>

It can simplified a lot rewriting it like:

<?php
function flexifilter_get_unused_delta() {
  $count = 0;
 
  foreach (flexifilter_get_filters(FALSE) as $flexifilter) {
    if (!$flexifilter['enabled']) {
      return $count;
    }
    $count++;
  }
 
  return FALSE;
}
?>

There is no reason to build an array of 128 items which are first set to TRUE, and then (some of them) to FALSE, when the function returns a single value. This means to use 3 loops against 1 that is really needed.

Login or register to post comments

#9

kiamlaluno - November 13, 2008 - 20:17

In flexifilter_filter_edit_components_data() there is the following code:

<?php
  foreach ($components as $n => $component) {
    $us = array();
    foreach ($component['settings'] as $key => $value) {
      if ($key == 'components') {
        $us['components'] = flexifilter_filter_edit_components_data($value, $id_prefix);
      }
      elseif ($key == 'condition') {
        $us['condition'] = flexifilter_filter_edit_condition_data($value);
      }
      elseif ($key == 'step') {
        $us[$key] = $value;
      }
      else {
        $us['setting_'. $key] = $value;
      }
    }
?>

It can better rewritten like:

<?php
  foreach ($components as $n => $component) {
    $us = array();
    foreach ($component['settings'] as $key => $value) {
      switch($key) {
        case 'components':
          $us['components'] = flexifilter_filter_edit_components_data($value, $id_prefix);
          break;
        case 'condition':
          $us['condition'] = flexifilter_filter_edit_condition_data($value);
          break;
        case 'step':
          $us[$key] = $value;
          break;
        default:
          $us['setting_'. $key] = $value;
          break;
      }
    }
  }
?>

Login or register to post comments
 
 

Drupal is a registered trademark of Dries Buytaert.