2
\$\begingroup\$

I'm beginning with PHP programming and would appreciate a critique on my code and how I could make it more efficient, rather than repeating the if statements.

What I'm basically doing is getting the fields typed in by the user and displaying these fields in a sidebar of a custom page template.

<?php


            $name = (get_field('name'));
            $address = (get_field('address'));
            $business= (get_field('company'));
            $phone= (get_field('phone'));
            $fax= (get_field('fax'));
            $mobile= (get_field('mobile'));
            $email =  (get_field('email'));
            $personalemail =  (get_field('second_email'));
            $website =  (get_field('website'));
            $facebook =  (get_field('facebook'));
            $twitter =  (get_field('twitter'));
            $linkedin =  (get_field('linkedin'));

            if (!empty($name)) {
                echo $name . "<br />";
            }

            if (!empty($business)) {
                echo $business . "<br />";
            }

            if (!empty($address)) {
                echo $address . "<br />";
            }

            if (!empty($phone)) {
                echo $phone . "<br />";
            }

            if (!empty($fax)) {
                echo $fax . "<br />";
            }

            if (!empty($mobile)) {
                echo $mobile . "<br />";
            }

            if (!empty($email)) {
                echo "<a href=\"mailto:$email\">$email</a>" . "<br />";
            }

            if (!empty($personalemail)) {
                echo "<a href=\"mailto:$personalemail\">$personalemail</a>" . "<br />";
            }

            if(!empty($website)){
                $final_url = (!preg_match("~^(?:f|ht)tps?://~i", $website))? 'http://'.$website: $website;
                echo "<a href=\"$final_url\">$final_url</a>" . "<br />";

            }

            if(!empty($facebook)){
                $final_url = (!preg_match("~^(?:f|ht)tps?://~i", $facebook))? 'http://'.$facebook: $facebook;
                echo "<a href=\"$final_url\"><img src=\"/wp-content/uploads/2013/05/fb1.png\"></a>";

            }

            if(!empty($twitter)){
                $final_url = (!preg_match("~^(?:f|ht)tps?://~i", $twitter))? 'http://'.$twitter: $twitter;
                echo "<a href=\"$final_url\"><img src=\"/wp-content/uploads/2013/05/tw1.png\"></a>";

            }

            if(!empty($linkedin)){
                $final_url = (!preg_match("~^(?:f|ht)tps?://~i", $linkedin))? 'http://'.$linkedin: $linkedin;
                echo "<a href=\"$final_url\"><img src=\"/wp-content/uploads/2013/05/linkedin.png\"></a>";

            }

            ?>
\$\endgroup\$

3 Answers 3

3
\$\begingroup\$

Your code can be simplified a bit. The parantheses on the right side is not needed. Instead of

        $name = (get_field('name'));

you can just say

        $name = get_field('name');

However, since you are outputting the values directly, I'd use another approach for preparation. Collect all output strings in an array:

        $result = array();
        $result[] = get_field('name');
        $result[] = get_field('address');
        $result[] = get_field('company');
        $result[] = get_field('phone');
        $result[] = get_field('fax');
        $result[] = get_field('mobile');
        $result[] = mailTo(get_field('email'));
        $result[] = mailTo(get_field('second_email'));
        $result[] = linkTo(get_field('website'));
        $result[] = linkTo(get_field('facebook'), '<img src="/wp-content/uploads/2013/05/fb1.png">');
        $result[] = linkTo(get_field('twitter'), '<img src="/wp-content/uploads/2013/05/tw1.png">');
        $result[] = linkTo(get_field('linkedin'), '<img src="/wp-content/uploads/2013/05/linkedin.png">');

The functions mailTo and linkTo will build the links; I'll describe them later. The point is, that now you have a single array with all lines. Some of the lines will be empty, because no data was provided. You can filter those empty lines easily with

         $result = array_filter($result);

To show all the lines with a <br /> in between, you just implode the array into a single string and echo it:

         echo implode('<br />', $result);

Now your main functionality is done, and the only thing left is to put the formatting code, that you used repeatedly, into proper functions.

function mailTo($address)
{
    if (isValidEmailAddress($address)) {
        return printf('<a href="mailto:%1$s">%1$s</a>', $address);
    } else {
        return '';
    }
}

With this function, you have one single place, where the rendering of an email address is defined, so, if you want to change the appearance (eg. link it to a contact page instead of using mailto), you only have to that once.

Here a new function is introduced to check the validity of the email address. To keep it close to what your original code did, for now we'll only check if it is non-empty.

function isValidEmailAddress($address)
{
    $address = trim($address);
    return !empty($address);
}

You can of course add other validations to it.

The same has to be done to URLs.

function linkTo($url, $linkText = null)
{
    $url = trim($url);
    if (empty($url)) {
        return '';
    }

    if (is_null($linkText)) {
        $linkText = $url;
    }

    if (!preg_match("~^(?:f|ht)tps?://~i", $url)) {
        $url = 'http://' . $url;
    }

    return sprintf('<a href="%s">%2</a>', $url, $linkText);
}

As you can see, putting repetitive code sequences into their own functions increases the readability of your code and make future changes easier.

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

I'll add another approach to the nice nibra's answer, highlighting his notice about reusable functions.

AFAIK, the function get_field() is from Advanced Custom Fields, but let's make some helper data and function for a self-contained example:

# Sample data
$sample_values = array( 
    'name'          => 'Sofia Lorem Ipsum', 
    'address'       => 'Italy', 
    'company'       => 'Cinecita', 
    'phone'         => '+00 00 00', 
    'fax'           => '+00 11 11', 
    'mobile'        => '+00 22 22', 
    'email'         => '[email protected]', 
    'second_email'  => '[email protected]', 
    'website'       => 'sofia.it', 
    'facebook'      => 'sofia-lorem-ipsum', 
    'twitter'       => 'sofipsum', 
    'linkedin'      => 'Sofia The Great', // This last comma doesn't hurt and prevent errors when we add new lines copying this or any previous one
);
# Sample function
function get_field( $name )
{
    global $sample_values; // globals are not really recommended, better use OOP
    return $sample_values[ $name ];
}

The data you need to grab and loop into can be classified in 3 types, so let's make another associative array out of it:

# List of registered fields
$fields = array( 
    'name'          => 'simple', 
    'address'       => 'simple', 
    'company'       => 'simple', 
    'phone'         => 'simple', 
    'fax'           => 'simple', 
    'mobile'        => 'simple', 
    'email'         => 'email', 
    'second_email'  => 'email', 
    'website'       => 'url', 
    'facebook'      => 'url', 
    'twitter'       => 'url', 
    'linkedin'      => 'url',
);

Now we loop and call our controller function, sending the name of the field, its type and its value (got with get_field()).

# Loop, get value and print
foreach( $fields as $name => $type ) // $key=>$value pair from an associative array
{
    $value = get_field( $name );
    my_print_field( $name, $type, $value );
}

Finally, the print function. Note the use of printf() and check how the format (%s, %1$s) works in sprintf page. Instead of printf, you can use calls to custom functions to do validation like explained by nibra.

# Custom print function
function my_print_field( $name, $type, $value ) // unique prefix always, can be avoided with OOP 
{
    switch( $type )
    {
        case 'simple':
            printf(
                '%s is <b>%s</b><br />',
                $name,
                $value
            );
        break;
        case 'email': 
            // or my_mail_to( $name, $value );
            printf(
                '%1$s is <b><a href="mailto:%2$s">%2$s</a></b><br />',
                $name,
                $value
            );
        break;
        case 'url': 
            // or my_url( $name, $value );
            $final_url = (!preg_match("~^(?:f|ht)tps?://~i", $value))? 'http://'.$value: $value;
            printf(
                '%1$s is <b><a href="%2$s">%2$s</a></b><br />',
                $name,
                $final_url
            );
        break;
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Would this not be more efficient to do it like this... $postFields = get_fields(get_the_ID()); # Loop, get value and print foreach( $fields as $name => $type ) // $key=>$value pair from an associative array { $value = $postFields[$name]; my_print_field( $name, $type, $value ); } that way you only fetch the field values once and dont have to do the continuous queries \$\endgroup\$
    – Clark T.
    Dec 10, 2013 at 14:24
1
\$\begingroup\$

My suggestion would be a slight modification on @brasofilo's answer

# List of registered fields
$fields = array(
    'name'         => 'simple',
    'address'      => 'simple',
    'company'      => 'simple',
    'phone'        => 'simple',
    'fax'          => 'simple',
    'mobile'       => 'simple',
    'email'        => 'email',
    'second_email' => 'email',
    'website'      => 'url',
    'facebook'     => 'url',
    'twitter'      => 'url',
    'linkedin'     => 'url',
);

This way you only fetch the fields once for the post instead of querying the database for each and every field.

$postFields = get_fields(get_the_ID());

# Loop, get value and print
foreach ($fields as $name => $type) // $key=>$value pair from an associative array
{
    my_print_field($name, $type, $postFields[$name]);
}

Custom print function

function my_print_field($name, $type, $value) // unique prefix always, can be avoided with OOP 
{
    switch ($type) {
        case 'simple':
            printf(
                '%s is <b>%s</b><br />',
                $name,
                $value
            );
            break;
        case 'email':
            // or my_mail_to( $name, $value );
            printf(
                '%1$s is <b><a href="mailto:%2$s">%2$s</a></b><br />',
                $name,
                $value
            );
            break;
        case 'url':
            // or my_url( $name, $value );
            $final_url = (!preg_match("~^(?:f|ht)tps?://~i", $value)) ? 'http://' . $value : $value;
            printf(
                '%1$s is <b><a href="%2$s">%2$s</a></b><br />',
                $name,
                $final_url
            );
            break;
    }
}
\$\endgroup\$

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.