Creating an array that loops out on the frontend - Don't Repeat Yourself?

Question

Entire Code Here

Don't repeat yourself

I know of the "Don't repeat yourself". Still it's getting messy in some places. The code below contains three blocks of code.

They are similar in some ways and different in others. It doesn't follow the same pattern in all ways.

What it does

It creates an array that I loop out on the frontend.

Question

Is there any better way to do this kind of stuff? Less messy, better structure?

$message_start = '<strong>h2 tags</strong> - ';
$message_end = '<span class="counter">' . $this->count_h2 . '</span>';
if( $this->count_h2 == 0 ) {
    $message = 'No tags found. Add some!';
    $array['content_editor']['count_h2']['status'] = 2;
} elseif( $this->count_h2 == 1 ) {
    $message = 'Some found. Too few!';
    $array['content_editor']['count_h2']['status'] = 1;
} else {
    $message = 'Many found. Great!';
    $array['content_editor']['count_h2']['status'] = 0;
}
$array['content_editor']['count_h2']['message'] = $message_start . $message . $message_end;
$array['content_editor']['count_h2']['count'] = $this->count_h2;

$message_start = '<strong>h3-h6 tags</strong> - ';
$h2_h6 = $this->count_h3 + $this->count_h4 + $this->count_h5 + $this->count_h6;
$counter = ( $h2_h6 == 0 ) ? '' : $h2_h6;
$message_end = '<span class="counter">' . $counter . '</span>';
if( $h2_h6 == 0 ) {
    $message = 'No found. Add some!';
    $array['content_editor']['count_h3_h6']['status'] = 1;
} else {
    $message = 'Found, great!';
    $array['content_editor']['count_h3_h6']['status'] = 0;
}
$array['content_editor']['count_h3_h6']['message'] = $message_start . $message . $message_end;
$array['content_editor']['count_h3_h6']['count'] = $this->h2_h6;

$message_start = '<strong>Title keywords</strong> - ';
$counter = ( $this->found_keywords1_post_title == 0 ) ? '' : $this->found_keywords1_post_title;
$message_end = '<span class="counter">' . $counter . '</span>';
if( count( $this->keywords1 ) == 0 ) {
    $message = 'No primary added.';
    $array['content_editor']['missing_keywords1_post_title']['status'] = 2;
} elseif( $this->found_keywords1_post_title == 0 ) {
    $message = 'No primary found.';
    $array['content_editor']['missing_keywords1_post_title']['status'] = 2;
} else {
    $s = ( $this->found_keywords1_post_title != 1 ) ? 's' : '';
    $message = 'Primary found.';
    $array['content_editor']['missing_keywords1_post_title']['status'] = 0;
}
$array['content_editor']['missing_keywords1_post_title']['message'] = $message_start . $message . $message_end;
$array['content_editor']['missing_keywords1_post_title']['count'] = $this->found_keywords1_post_title;

Answer 1

All the other guys are in general right, so I will skip this part. If you just want to reduce code duplication and as a side effect increase testability and readability please have a look at the following code.

I guess there is no doubt, that you would benefit from an object containing the status, message and count as they somehow belong together.

<?php
$f=new Foo();
var_dump($f->foo());

class Foo
{
    private $count_h2=1;
    private $count_h3=1;
    private $count_h4=0;
    private $count_h5=0;
    private $count_h6=1;
    private $keywords1=2;
    private $found_keywords1_post_title=3;

    public function foo()
    {
        list($status,$message,$count)=$this->evaluateH2Count();
        $this->storeMessage($array,'count_h2',"h2 tags", $count, $message, $status);
        list($status,$message,$count)=$this->evaluateHXCount();
        $this->storeMessage($array,'count_h3_h6',"h3-h6 tags", $count, $message, $status);
        list($status,$message,$count)=$this->evaluateKeywordCount();
        $this->storeMessage($array,'missing_keywords1_post_title',"Title keywords", $count, $message, $status);
        return $array;
    }

    private function evaluateH2Count()
    {
        if( $this->count_h2 == 0 ) {
            $message = 'No tags found. Add some!';
            $status = 2;
        } elseif( $this->count_h2 == 1 ) {
            $message = 'Some found. Too few!';
            $status = 1;
        } else {
            $message = 'Many found. Great!';
            $status = 0;
        }
        return array($status,$message,$this->count_h2);
    }

    private function evaluateHXCount()
    {
        $h2_h6 = $this->count_h3 + $this->count_h4 + $this->count_h5 + $this->count_h6;
        if( $h2_h6 == 0 ) {
            $message = 'No found. Add some!';
            $status = 1;
        } else {
            $message = 'Found, great!';
            $status = 0;
        }
        return array($status,$message,$h2_h6);
    }

    private function evaluateKeywordCount()
    {
        if( count( $this->keywords1 ) == 0 ) {
            $message = 'No primary added.';
            $status = 2;
        } elseif( $this->found_keywords1_post_title == 0 ) {
            $message = 'No primary found.';
            $status = 2;
        } else {
            $message = 'Primary found.';
            $status = 0;
        }
        return array($status,$message,$this->found_keywords1_post_title);
    }

    //encapsulate the array access
    private function storeMessage(&$array,$key,$title,$count,$message,$status)
    {
        $array['content_editor'][$key]['message'] =  $this->formatMessage($title, $count, $message);
        $array['content_editor'][$key]['count'] = $count;
        $array['content_editor'][$key]['status'] = $status;
    }

    private function formatMessage($title,$count,$message) // actually moved to a template
    {
        $count=$count==0?'':$count; //assuming this is a bug in your h2 handling
        $message=$this->pluralize($message); //anything for template engine offers for handling appending Ss etc.
        return "<strong>$title</strong> - $message <span class=\"counter\">$count</span>";
    }

    //dummy
    private function pluralize ($message)
    {
        return $message;
    }

}

Answer 2

Architecturally, I'm adverse to hard-coding HTML tags within PHP code. If, for example, you wanted to migrate the code base from PHP to another language (Java, Python, Ruby, Go, Haxe, whatever) then you have to rewrite both the logic (PHP) and the presentation (PHP) code.

Consider decoupling the logic from the presentation so that you can change how the data appears without having to change how the data is retrieved. This simplifies switching implementation languages and opens the possibility to create different views of the same data for different devices.

To do this, you have to create an interface or contract between the logic and the system that renders the logic for the user. Fortunately, this is where XML excels.

Instead of hard-coding <strong>, you would create an XML document that represents the information you want to display. The XML document becomes the contract between the logic and presentation, which allows both to vary independently. Once the XML document is in place you can pass it to an XSLT Processor to generate the desired HTML content.

This offers the following advantages:

• Decoupled. The presentation logic moves to XSLT, completely decoupling it from the PHP code.
• Migration. Migrating from PHP to another language means the presentation logic need not change.
• Devices. It becomes trivial to change the presentation for different devices (use a new template).
• DRY. With display logic in a central location, duplicated code is easier to eliminate.
• Databases. Many databases now allow constructing XML documents within stored procedures, eliminating complex code readily expressed in SQL.
• SaaS. It simplifies creating a SaaS API -- transforming XML to JSON is fairly easy a solved problem.

This has the following disadvantages:

• Effort. Migrating to an XML/XSLT solution for presentation takes a lot of work.
• Knowledge. Introduces a second programming language (XSLT) into the software.
• Developers. Far fewer developers know XSLT than PHP.
• XSLT 1.0. Using PHP, the processor is limited to XSLT v1.0 (many great features are in XSLT 2.0).
• Bugs. The XSLT Processor has some pesky known bugs, which could be avoided by moving to XSLT 2.0 with the SAXON processor.

Answer 3

There's duplication between $message and $array['content_editor']['count_h2']['status'] in the first block and its counterpoints in the other blocks - they both convey the same information. Instead of using $message you can use $array['content_editor']['count_h2']['status'] as an index into a constant array of messages.

Answer 4

Here is my take on it...

<style type="text/css">
.section-title{
    font-weight: bold;
}
    
</style>

<?php 
    $h2_array['message'] = '<span class="section-title">h2 tags</span> - ';

    switch ($this->count_h2) {
        case 0:
            $h2_array['message'] .= 'No tags found. Add some!';
            $h2_array['status'] = 2;
            break;
        case '1':
            $h2_array['message'] .= 'Some found. Too few!';
            $h2_array['status'] = 1;
            break;
        default:
            $h2_array['message'] .= 'Many found. Great!';
            $h2_array['status'] = 0;
            break;
    }
    
    $h2_array['message'] .= '<span class="counter">' 
                         . $this->count_h2 
                         . '</span>';

    $h2_array['count'] = $this->count_h2; // Why?

    

    // BIG COMMENT SERVES AS A VISUAL DIVIDER OF SECTIONS
    
    $h3_h6_array['message'] = '<span class="section-title">h3-h6 tags</span> - ';
    
    $h3_h6_array['count'] = $this->count_h3 
                          + $this->count_h4 
                          + $this->count_h5 
                          + $this->count_h6;

    // Realizing there are only two options,
    // this should be an if statement, not a switch.
    switch ($h3_h6_array['count']) {
        case 0:
            $h3_h6_array['message'] .= 'No found. Add some!';
            $h3_h6_array['status'] = 1;
            break;
        default:
            $h3_h6_array['message'] .= 'Found, great!';
            $h3_h6_array['status'] = 0;
            break;
    }

    if ($h3_h6_array['count']) 
    {
        $h3_h6_array['message'] .= '<span class="counter">' 
                                . $h3_h6_array['count'] 
                                . '</span>';
    }
?>

Now, I only did the first two sections, but I think you get the idea and can repeat it on the third. I echo other comments here about mixing presentation with logic, but I also recognize you are just getting into coding and sometimes practical application is more important.

What I did:

• I cleaned up the arrays. Even with autocomplete in my IDE, typing all those array names was a nightmare. It doesn't have to be so complicated.

• I got rid of quite a few unnecessary variables. Message start and end just didn't make sense. Variables are values you want to reuse. But you were only using them one time. I also moved them so they were assigned in the order of the final output. This just gave it a more logical flow to me.

• I switched your strong tags to spans with classes.

A few techniques to note:

1. Comment your code. Don't be afraid to be verbose. Write what you were thinking when you wrote the code and why you make specific decision. That reference will help you tremendously down the road.

2. When you use an if statement, know that false, null, 0, empty strings and empty arrays will ALL will equate to false. So, instead of if($this->count_h2 == 0){ ... } you can just write if($this->count_h2){ ... }.

   Now, I don't know what your count_h2 method returns. Many methods/functions will return false or null when there is an error, in which case NEITHER of the above methods would work. If you want to make sure you are testing for the numeral zero and not for the boolean value false, you need to use three equal signs: if($this->count_h2 === 0){ ... }

3. When you echo all of this to the browser, it's going to look ugly and run together on a single line, making it difficult to read. Let me share with you a method I wish more php developers would use. It makes the output a bit prettier, without putting \n chars all through your code:

---

<?php
// Make an array, adding a new value for each line you want to print to the browser.
$message[] = '<span class="section-title">Title keywords</span> - ';
$message[] = 'No primary added.';
$message[] = '<span class="counter">' . $count . '</span>';

// Echo using implode, which joins each item in the array together to form a string. Each item is separated by a "new line".
echo implode("\n", $message);
?>

I hope that helps some. Good luck to you and keep learning. You will hit a point where it all gets much easier.