Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am used to procedural PHP. However, I have a piece of code that needs to be repeated a lot. I was thinking of trying my hand at creating a class. All is going well and it is working, but I think it can be done better. Especially the function where I am creating the HTML. At the moment, I have a variable $html and I keep adding data to it. It feels clunky and I wanted to know if anyone knows of a better solution.

I have been spending hours on Google trying to find a better way of doing it. Has anyone got advice or a link to a good tutorial?

Thank you in advance. Here is the class.

class clerksection {
    var $header;
    var $post;
    var $meta;

    function __construct($post, $meta, $header) {
        $this->post = $post;
        $this->meta = $meta;
        $this->header = $header;
    }

    //PHP magic methods to get values
    public function __get($property) {
        if (property_exists($this, $property)) {
            return $this->$property;
        }
    }

    //PHP magic methods to set values
    public function __set($property, $value) {
        if (property_exists($this, $property)) {
            $this->$property = $value;
        }
        return $this;
    }

    function create_html () {
        $repeat_group = get_post_meta( $this->post, $this->meta, true );
        $uid = spl_object_hash($this);
        $html = '<h2><strong>'. $this->header .'</strong></h2>';
        $html .= '<div class="row">';
        $i = 1;
        foreach( (array) $repeat_group as $key => $value ) :
            $html .= '<div class="col-lg-4 pd_left_15">';
            $html .= '<div class="pd_btm_20">';
            if (!empty($value['image'])) :
            $html .= '<img src="' . $value['image'] . '" class="img-responsive"/>';
            endif;
            $html .= '<p class="clerk_thick"><strong>' . $value['title'] . '</strong></p>';
            $html .= '<p class="dark_color clerk_thin"><strong>' . $value['name'] . '</strong>';
            if (!empty($value['notes'])) :
                $html .= '<a role="button" data-toggle="collapse" href="#' . $uid . $i . '" aria-expanded="false" aria-controls="' . $uid . $i . '">(more)</a>';
            endif;
            $html .= '</p>'; //.dark_color .clerk_thin
            if (!empty($value['notes'])) :
                $html .= '<p class="clerk_thin collapse" id="' . $uid . $i . '">' . $value['notes'] .'</p>';
            endif;
            if (!empty($value['phone'])) :
            $html .= '<p class="clerk_thin"><i class="fa fa-phone dark_color"></i> ' . $value['phone'] . '</p>';
            endif;
            if (!empty($value['email'])) :
            $html .= '<p class="clerk_thin"><i class="fa fa-envelope-o dark_color"></i> <a href="mailto:' . $value['email'] . '" target="_top"> ' . $value['email'] . '</a></p>';
            endif;
            $html .= '</div>'; //.pd_btm_20
            $html .= '</div>'; //.col-lg-4 .pd_left_15
            $i++;
        endforeach;
        $html .= '</div>'; //.row

        return $html;
    }
}
share|improve this question
    
you must ask this on StackOverflow, not on CodeReview – Bogdan Mart Jun 20 at 15:53
2  
My answer would be: Don't. A class represents data, or a task in your application: a record from the DB can be represented by a class. Another class might be used to read CSV files (and create instances of a DTO from that data). But a class like yours violates the SRP (single responsibility principle): it contains both data, and is responsible for the way that data is going to presented. Use templates for rendering, classes for logic and data representations. Don't combine the two, because the end-result will be messy, tightly coupled code, that is un-testable: anti-pattern central basically – Elias Van Ootegem Jun 20 at 16:14
    
Thanks for the feedback @EliasVanOotegem. I will use a template. – user1303840 Jun 21 at 7:57
    
@BogdanMart, I did ask it there first and they sent me here. – user1303840 Jun 21 at 7:57
    
@user1303840: While you're at it, there's a couple of things to take care of first: declaring properties using var was fine back in the days of PHP4, but the current stable version is PHP7, 7.1 isn't too far off either. Declare properties and methods the correct way: public function __construct(){} and protected $property. Also: don't rely on magic __set and __get if you can help it, use dedicated getters and setters. magic methods are slower, harder to test and more error prone – Elias Van Ootegem Jun 21 at 9:18

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Browse other questions tagged or ask your own question.