0

Here is the original script:

<?php
    if ( ! is_user_logged_in() ) {
        $args = array(
            'redirect'          => site_url( '/index.php/portal ' ), 
            'form_id'           => 'loginform-custom',
            'label_username'    => __( 'Username' ),
            'label_password'    => __( 'Password' ),
            'label_remember'    => __( 'Remember Me' ),
            'label_log_in'      => __( 'Log in' ),
            'remember'          => true
        );
        wp_login_form( $args );
    } else {
        wp_loginout( index.php/portal() );
    }
?>

I wanted to inject some HTML into this if/else script so I cut it up like this but I'm unsure if I did it right. It works when I run it but I care more if it's done right (because even poorly-written code can unfortunately work).

And here is how I cut it up.

<?php
    if ( ! is_user_logged_in() ) {
        $args = array(
            'redirect'          => site_url( '/index.php/portal ' ), 
            'form_id'           => 'loginform-custom',
            'label_username'    => __( 'Username' ),
            'label_password'    => __( 'Password' ),
            'label_remember'    => __( 'Remember Me' ),
            'label_log_in'      => __( 'Log in' ),
            'remember'          => true
        );
        wp_login_form( $args );
?>

<p>The portal is for registered users only.</p>

<?php
    } else {
        wp_loginout( index.php/portal() );
?>

<p>We recommend logging out after each session.</p>
<p><a href="/index.php/portal">Continue to portal</a></p>

<?php } ?>

So did I do it right?

1
  • Well... Does it work? Do you see your new paragraphs?
    – nibnut
    Commented Jan 27, 2017 at 21:05

1 Answer 1

1

Well-written as opposed to working but poorly written code is a distinct matter of opinion and taste. There are various style guides if you're interested in seeing examples.

Personally in your example above, I would change two things: indentation, and curly braces.

I find php open/closing tags in and html tags in templates to be hard to read when they are not lined up with the rest of the indentation:

<?php
if ( ! is_user_logged_in() ) {
    $args = array(
        'redirect'          => site_url( '/index.php/portal ' ), 
        'form_id'           => 'loginform-custom',
        'label_username'    => __( 'Username' ),
        'label_password'    => __( 'Password' ),
        'label_remember'    => __( 'Remember Me' ),
        'label_log_in'      => __( 'Log in' ),
        'remember'          => true
    );
    wp_login_form( $args );
    ?>

    <p>The portal is for registered users only.</p>

    <?php
} else {
    wp_loginout( index.php/portal() );
    ?>

    <p>We recommend logging out after each session.</p>
    <p><a href="/index.php/portal">Continue to portal</a></p>

<?php } ?>

This makes it much clearer what goes inside each if/else block. Along the same vein, I would replace the if/else curly braces with colons and endif tags, as well as move any php open/close tags onto the same lines as the php functionality.

<?php if ( ! is_user_logged_in() ) : ?>
    <?php 
    $args = array(
        'redirect'          => site_url( '/index.php/portal ' ), 
        'form_id'           => 'loginform-custom',
        'label_username'    => __( 'Username' ),
        'label_password'    => __( 'Password' ),
        'label_remember'    => __( 'Remember Me' ),
        'label_log_in'      => __( 'Log in' ),
        'remember'          => true
    );
    wp_login_form( $args ); ?>

    <p>The portal is for registered users only.</p>
<?php else: ?>
    <?php wp_loginout( index.php/portal() ); ?>

    <p>We recommend logging out after each session.</p>
    <p><a href="/index.php/portal">Continue to portal</a></p>
<?php endif; ?>

This makes it easier to see which if/else/end blocks are related to each other, as well ask making it clear what functionality is php and what is HTML.

Multi-line php blocks are somewhat of a code smell to me, indicating logic that should be put in a function or class elsewhere and referenced in a single line in your template:

<?php 
# some other file, maybe have a modules/ or helpers/ directory in your plugin
# or theme that contains functions and classes that are used for functionality.
function 5south_custom_login_form() {
    $args = array(
        'redirect'          => site_url( '/index.php/portal ' ), 
        'form_id'           => 'loginform-custom',
        'label_username'    => __( 'Username' ),
        'label_password'    => __( 'Password' ),
        'label_remember'    => __( 'Remember Me' ),
        'label_log_in'      => __( 'Log in' ),
        'remember'          => true
    );
    wp_login_form( $args );
}

# your template file
<?php if ( ! is_user_logged_in() ) : ?>
    <?php 5south_custom_login_form() ?>

    <p>The portal is for registered users only.</p>
<?php else: ?>
    <?php wp_loginout( index.php/portal() ); ?>

    <p>We recommend logging out after each session.</p>
    <p><a href="/index.php/portal">Continue to portal</a></p>
<?php endif; ?>

Generally I find that if there's more than 5-10 lines in a php if block in a template file, its usually worth breaking that functionality into a separate template and including it inside the if or else statement. This makes the template logic easier to follow.

If you find yourself nesting if/else statements in a complex way in a template file, it's probably time to reassess how the template file is loaded and perhaps create a function or functions to choose exact templates or partial templates for each specific situation.

Some people will see what I've done above and throw up in their mouth a little.

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.