3
\$\begingroup\$

I'm using WordPress ( PHP Based Platform ) and I'm trying to read an external XML File, convert it into a usable array, then compare two arrays - one from the xml file and another from an options database. I've coded it so that it does work but it just looks messy. I'm not sure if it's my naming conventions or how I'm formatting it but to me it looks lengthy and messy. I'm open to any / all suggestions on how to clean this up or make this function better.

The following function checks an XML file of private plugins then cross references it with the list of active plugins to make sure it's there.

/**
 * Get a list of Private Plugins from XML File
 * @return bool|array $plugins( 'name', 'version', 'path', 'slug', 'updated' )
 */
function getPrivate_plugins() {
    $plugins             = false;
    $active_plugins      = get_option( 'active_plugins' );
    $private_remote_path = 'http://domain.com/plugins.xml';
    $ci                  = curl_init( $private_remote_path );
    curl_setopt( $ci, CURLOPT_RETURNTRANSFER, 1 ); 
    $plugin_list         = curl_exec( $ci );
    curl_close( $ci );

    if( ! empty( $plugin_list ) ) {
        $plugin_obj   = simplexml_load_string( $plugin_list );
        $plugin_json  = json_encode( $plugin_obj );
        $plugin_array = json_decode( $plugin_json, TRUE );
        $plugin_array = ( ! empty( $plugin_array ) && isset( $plugin_array['plugin'] ) ) ? $plugin_array['plugin'] : '';

        if( ! empty( $plugin_array) ) {
            $plugin_array = array_map(
                function( $arr ) {
                    return ( isset( $arr['path'] ) ) ? $arr['path'] : '';
                }, 
                $plugin_array 
            );
            $plugins = array_intersect( array_filter( $plugin_array ), $active_plugins );
            $plugins = ( ! empty( $plugins ) ) ? $plugins : false;
        }
    }

    return $plugins;
}

The only WordPress based function is get_option() which return to $active_plugins an array of paths [0] => 'plugin-directory/plugin-file.php'

The XML File it's reading is simple:

<?xml version="1.0" encoding="UTF-8"?>
<plugins>
    <plugin>
        <name>Private Plugin A</name>
        <slug>private-plugin-a</slug>
        <path>private-plugin-a/private-plugin-a.php</path>
        <version>1.1</version>
        <updated>Feb 23, 2015 03:26pm</updated>
    </plugin>
    <plugin>
        <name>Private Plugin B</name>
        <slug>private-plugin-b</slug>
        <path>private-plugin-b/private-plugin-b.php</path>
        <version>1.2</version>
        <updated>Feb 24, 2015 12:26pm</updated>
    </plugin>
</plugins>
\$\endgroup\$
1
\$\begingroup\$

Types

Your code would be easier to read if one variable could only be one type. $plugins is initiated as a boolean, but can also be an array. But what would a boolean value even mean? $plugins = false; doesn't really make sense.

If there are no plugins, your code should just return an empty array. If there are errors loading the plugins, you could return false (but don't use the $plugin variable for that), or ideally throw an exception if you are using PHP5.

Don't reuse variables

You shouldn't reuse variables for completely different things. For example $plugin_array seems to be holding a lot of different data, which is quite confusing.

This for example:

    $plugin_array = json_decode( $plugin_json, TRUE );
    $plugin_array = ( ! empty( $plugin_array ) && isset( $plugin_array['plugin'] ) ) ? $plugin_array['plugin'] : '';

    if( ! empty( $plugin_array) ) {
        // do stuff
    }

Would be better as:

    $plugin_array = json_decode( $plugin_json, TRUE );
    $is_valid_data = ! empty( $plugin_array ) && isset( $plugin_array['plugin'] );
    $plugin = $plugin_array['plugin'];

    if( $is_valid_data ) {
        // do stuff
    }

Or you could just save the $is_valid_data variable by putting the expression inside the if directly.

Return early

Generally, your code will get a lot more readable if you handle simple checking at the top of your function, and return early. This will also reduce the level of nesting. eg:

if( empty( $plugin_list ) ) {
    return array();
}

And the same with $plugins = ( ! empty( $plugins ) ) ? $plugins : false;, which could just be:

if ( empty( $plugins ) ) {
    return array();
}

Or just ignore it, as there doesn't seem to be an error to handle (it's already empty, and returning an empty array if there are no plugins seems appropriate). Same goes for if( ! empty( $plugin_array) ) {.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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