1
\$\begingroup\$

I am working on a function that will take a an array of options. These options are retrived from a database, and could have other options inputted throughout the system. When adding my options, I am constrained by only being able to add a single option at a time. I can't currently add an array of options. The options that the system adds can be unpredictable, because of the codebase. The resulting array from the database call could look like this, except for the option keys I define starting with custom_, those I can rely on being there, since I set them:

array(
    0 => '...',
    'menu_items' => array(
        'item1' => '...',
        'item2' => '...',
        'item3' => '...',
        'item4' => '...',
    ),
    'another_item'                  => '...',
    'custom_contact_office_name'           => 'The Office Name',
    'custom_contact_office_address'        => '1234 Address St',
    'custom_contact_office_phone'          => '555.555.5555',
    'custom_contact_office_email'          => '[email protected]',
    'custom_contact_office_fax'            => '555.555.5555',
    'custom_contact_office_leader_name'    => 'Mr. Pickles',
    'custom_contact_office_leader_address' => '5678 Address St',
    'custom_contact_office_leader_phone'   => '555.555.5555',
    'custom_contact_office_leader_email'   => '[email protected]',
    'custom_social_linkedin'               => 'linkedin.com/profile_url',
    'custom_social_facebook'               => 'facebook.com/profile_url',
    'custom_social_twitter'                => 'twitter.com/profile_url',
    'custom_social_gplus'                  => 'google.com/plus/profile_url',
    'custom_social_github'                 => 'github.com/profile_url',
    'custom_social_youtube'                => 'youtube.com/profile_url',
    'another_option'                => '...',
    'some_other_kind_of_option'     => '...',
    'some_more_items_of_this'       => '...',
);

The way the system works is to serialize all the options in one array and save it to the db table. So you have to register the option by "id" which is really just used as the key in the massive serialized array that gets saved, so it needs to be unique, thus all the prefixes. I'm not interested in the other defined options, in fact, I want to skip over them while parsing through the array. So I wrote a couple functions to parse the office section:

/**
 * Parse the settings passed in
 *
 * @param $main_pattern
 * @param $sub_pattern
 * @param $option_name
 * @param $option_value
 *
 * @return null
 */
function setting_parser($main_pattern, $sub_pattern, $option_name, $option_value) {
    if (preg_match( $main_pattern, $option_name )) {
        if (preg_match( $sub_pattern, $option_name )) {
            return $option_value;
        }
    }
    return null;
}

/**
 * Apply the values to the multi-dimensional array
 * 
 * @param array  $theme_options
 * @param array  $info_types
 * @param string $section
 * @param string $section_pattern
 * @param bool  $is_option_string
 * 
 * @return array
 */
function apply_multi_dimensional_values(array $theme_options, array $info_types, $section, $section_pattern, $is_option_string) {
    $array_ref = array();
    foreach ($theme_options as $option_name => $option_value) {
        if ($is_option_string && preg_match( '/custom/', $option_name )) {
            /* Check if it's a string, to avoid numerically indexed values */
            if ( is_string( $option_name ) ) {
                if ($info_types !== null && is_array( $info_types )) {
                    for ($i = 0; $i<count( $info_types ); $i++) {
                        if (preg_match( sprintf( '/%s/', $info_types[ $i ] ), $option_name ) && preg_match( $section_pattern, $option_name )) {
                            $array_ref[$section][$info_types[$i]] = setting_parser( $section_pattern, sprintf( '/%s/', $info_types[$i] ), $option_name, $option_value );
                        }
                    }
                }
            }
        } else {
            if ($info_types !== null && is_array( $info_types ) && preg_match( '/custom/', $option_name )) {
                for ($i = 0; $i<count( $info_types ); $i++) {
                    if (preg_match( sprintf( '/%s/', $info_types[ $i ] ), $option_name ) && preg_match( $section_pattern, $option_name )) {
                        $array_ref[$section][$info_types[$i]] = setting_parser( $section_pattern, sprintf( '/%s/', $info_types[$i] ), $option_name, $option_value );
                    }
                }
            }
        }
    }
    return $array_ref;
}
/**
 * Get the office info
 *
 * @param array $theme_options
 * @param bool  $is_option_string
 *
 * @return array
 * @throws Exception
 */
function get_office_info(array $theme_options, $is_option_string = true) {
    $info_types = array(
        'name',
        'address',
        'phone',
        'email',
        'fax'
    );
    // if leader does NOT come after office, but it in office array
    // uses positive look-ahead
    $section_pattern = '/office_(?!leader)/';
    $section = 'office';

    if (empty( $theme_options )) throw new Exception("First argument MUST be a valid array.");

    $office_info = apply_multi_dimensional_values( $theme_options, $info_types, $section, $section_pattern, $is_option_string );

    return $office_info;
}

/**
 * @param array $theme_options
 * @param bool  $is_option_string
 *
 * @return array
 * @throws Exception
 */
function get_leader_info(array $theme_options, $is_option_string = true) {
    $info_types = array(
        'name',
        'address',
        'phone',
        'email'
    );
    // If leader comes after office, put it in leader array
    // uses negative look-ahead
    $section_pattern = '/(office_(?=leader))/';
    $section = 'leader';

    if (empty( $theme_options )) throw new Exception("First argument MUST be a valid array.");

    $leader_info = apply_multi_dimensional_values( $theme_options, $info_types, $section, $section_pattern, $is_option_string );

    return $leader_info;
}

The purpose of parsing these arrays is to basically group them into the relevant data. I group them by a section key that I pass into both the get_office_info() function and the get_leader_info() function. The bit of code handling this looks like this:

$array_ref[$section][$info_types[$i]] = ...

So that the result of calling get_office_info() will be an array like this:

'office' => array(
    'name'    => 'The Office Name',
    'address' => '1234 Address St',
    'phone'   => '555.555.5555',
    'email'   => '[email protected]',
    'fax'     => '555.555.5555'
)

The result of calling get_leader_info() will be an array like this:

'leader' => array(
    'name'    => 'Mr. Pickles',
    'address' => '5678 Address St',
    'phone'   => '555.555.5555',
    'email'   => '[email protected]'
)

I just can't help but feeling like there's a cleaner way to do this that I am missing, or don't have enough experience to recognize. I am doing a similar thing with social, but it's different because it will be a single-dimensional, numerically indexed array containing a social profile link that I can just loop through. If the clean-up for this solution doesn't kill both birds, I will post that as a seperate code review.

The calling goes like this get_(office|leader)_info() -> apply_multi_dimensional_values() -> setting_parser() which will get called in a loop.

The part in particular that I am concerned about is all the logic inside the apply_multi_dimensional_values() function. Please let me know if you need any more information to review the code.

\$\endgroup\$
2
  • \$\begingroup\$ Are you sure that your code do what you want it to do?. There are too many strange things. \$\endgroup\$
    – Arnial
    Oct 27, 2016 at 15:41
  • \$\begingroup\$ The underlying problem here is that you're using arrays for everything. Use objects for starters. Then, address how the data is being fetched. Clearly, the data is returned/loaded in a format that requires intense processing just to get it to a usable state. Rather than re-formatting the data, why not fix the data source? \$\endgroup\$ Nov 2, 2016 at 18:05

3 Answers 3

1
\$\begingroup\$

I think It will be much easer to get required options if you

  1. Create array with keys that you need (where key is full key name in theme_options).
  2. Then walk over theme_options and fill values in these arrays.
  3. Remove unneeded prefix from keys in arrays.

Check this:

/**
 * @param string $prefix
 * @param array $endings
 * @param array $theme_options
 */
function find_opts_with_prefix( $prefix, array $endings, array $theme_options ){
    /*
      return array with theme options which key is equal to $prefix.$ending (for each ending in enginds).
     */

    $required_keys = Array(); //create keys array
    foreach( $endings as $end ){
        $required_keys[ $prefix . $end ] = null;
    }

    foreach( $theme_options as $key => $value ){ //fill value if key exists
        if( array_key_exists( $required_values[ $key ] ) )
            $required_values[ $key ] = $value;
    }

    $not_null = function( $value ){ return $value !== null; } ;
    return array_filter( $required_values, $not_null ); //filter out null values
}

/**
 *  @param string $prefix
 *  @param array $options
 */
function remove_prefix( $prefix, array $options ){
    /*
      remove prefix form every option  
      return new array with options without prefix
     */
    $result_opts = Array();
    $prefix_len = str_len( $prefix );
    foreach( $options as $key => $value ){
        $result_opts[ substr( $key, $prefix_len ) ] = $value;
    }
    return $result_opts;
}


/**
 * @param string $prefix
 * @param array $info_types
 * @param array $theme_options
 */
function get_infos( $prefix, array $info_types, array $theme_options) {

    $found_types = find_opts_with_prefix( $prefix, $info_types, $theme_options );
    return remove_prefix( $fountd_types );
}

function is_custom_option( $option_name ){
    return strncmp( $option_name, 'custom_', 7 );
}

function group_options( array $theme_options ){
    /* 
       This method will build your 
       $array_ref[$section][$info_types[$i]] 
    */

    if (empty( $theme_options )) 
         throw new Exception("First argument MUST be a valid array.");

    //define sections and info types.
    $info_types = Array( 'offise' => Array( 'name',
                                            'address',
                                            'phone',
                                            'email',
                                            'fax' ),

                         'leader' => Array( 'name',
                                            'address',
                                            'phone',
                                            'email' ) );

    // prefixes, like "custom_contact_office", and sections 
    // better put into array with info_types.
    // but i was too lazy.

    $custom_opts = array_filter( $theme_options, 'is_custom_option' );

    $array_refs = Array();
    $array_refs['office'] = get_infos( 'custom_contact_office', $info_types['office'], $custom_opts );
    $array_refs['leader'] = get_infos( 'custom_contact_office_leader', $info_types['leader'], $custom_opts );
    return $array_refs;
}
\$\endgroup\$
1
  • \$\begingroup\$ This answer helped me the best. I dropped this into PHPFiddle and after working through the PHP errors, which I expected with example code, I just wanted to point something out I ran into: is_custom_option(), which is the callback for array_filter() was getting passed the value of the array, not the key. But as of PHP 5.6 they introduced the ARRAY_FILTER_USE_KEY as well as ARRAY_FILTER_USE_BOTH, which helped. Thank you for your review, I learned a lot! A lot more elegant than my solution. \$\endgroup\$
    – mrClean
    Oct 27, 2016 at 20:38
2
\$\begingroup\$
  1. Redundant is_string check.

    if ($is_option_string && preg_match( '/custom/', $option_name )) {
        /* Check if it's a string, to avoid numerically indexed values */
          if ( is_string( $option_name ) ) {  // <==
    

    If variable option_name matches with /custom/ then it is string.

  2. Inner loops are same: extract code to function, and use foreach instead of for. (This is first 'strange' part of your code).

  3. After code extraction, you will see that first two if's are useless, parameter is_option_string don't do anything. (Second strange part).

    // this code    
    if ($is_option_string && preg_match( '/custom/', $option_name )) {
            if ($info_types !== null && is_array( $info_types )) {
                  /*call extracted func here */
            }
    } else {
            if ($info_types !== null && is_array( $info_types ) && preg_match( '/custom/', $option_name )) { /*extracted func*/ }
    }
    
    // is equivalent to
    
    $info_is_ok = $info_types !== null && is_array( $info_types );
    $match_exists = preg_match( '/custom/', $option_name );
    if ($is_option_string && $match_exists && $info_is_ok ){
        /*call extracted func*/ 
    } elseif ( $info_is_ok && $match_exists) { 
        /*call extracted func*/ 
    }
    //doesn't metter what in $is_option_string, 
    //extracted function will be called if match exists and info_type is ok.
    
  4. If info_type is null or not Array then foreach loop won't do anything

\$\endgroup\$
1
\$\begingroup\$

I question the whole persistence model. Rather than explore ways to write code to work around what looks to be a bad data storage model, I would think about how you can best persist the data in an appropriate manner such that it can be read into your application without an intervening layer of regex-based data mapping.

\$\endgroup\$
2
  • \$\begingroup\$ +1 Yeah, you and me both! However, with legacy systems that's not always the solution the bosses are looking for in the middle of a project. Best to re-visit it later. \$\endgroup\$
    – mrClean
    Oct 27, 2016 at 20:47
  • 1
    \$\begingroup\$ @mrclean208: Bosses sometimes need devs to tell them that, if they don't want to be faced with issues in the future (ie clients being unhappy because of bugs, potential loss of revenue as a result, and lots of hours wasted on refactoring), that it's worth it to bite the bullet earlier rather than later. I honestly believe that you're trying to find a solution for a problem that shouldn't exist. Fix problems at their source... \$\endgroup\$ Nov 2, 2016 at 18:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.