The code needs to be optimized.
Code like

  if ($value == TRUE) {

can be rewritten like

  if ($value) {

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

Comments

avpaderno’s picture

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

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

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

  return $conf_backup;
}
avpaderno’s picture

Also in flexifilter_get_component_list() the following code is used:

    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:

  $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:

    $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]);
      }
    }
avpaderno’s picture

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

  static $cache;
  if (!isset($cache)) {
    $cache = array();
  }

can be simplified in

  static $cache = array();
steven jones’s picture

Good points, but, provide a patch please.

avpaderno’s picture

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.

avpaderno’s picture

In _flexifilter_filter_from_db_row() there is the following lines of code

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

which can be rewritten like

    '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

  $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

  $filter = $row;

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

avpaderno’s picture

flexifilter_get_number_enabled_filters() use the following code:

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:

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;
}
avpaderno’s picture

The code of flexifilter_get_unused_delta() is the following:

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:

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.

avpaderno’s picture

In flexifilter_filter_edit_components_data() there is the following code:

  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:

  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;
      }
    }
  }
avpaderno’s picture

Version: 6.x-1.1-rc1 » 6.x-1.x-dev
Issue summary: View changes
Status: Active » Closed (outdated)

I am closing this issue since it's for a Drupal version that isn't supported.