4
\$\begingroup\$

I'm busy on a site, and we've made a custom Twitter class. The tweets are send from our panel, where they're posted and saved in the DB. In this class, we get the tweets from the database and show them on our site.

These are my helping points:

  • Is this code as efficient as possible?
  • Is there a easier way to achieve the same result?
  • Is there a faster way to achieve the same result?

<?php 
class Tweets 
{ 
    private $tweetData = array(); 
    private $ctweet = 1; 

    private function fetchTweets() 
    { 
        $stmt = $db->connection->prepare('SELECT * FROM tweet ORDER BY id DESC LIMIT 5'); 
        $stmt->execute(); 
        $result = $stmt->get_result(); 
        if ($stmt->num_rows > 0) 
        { 
            while ($row = $result->fetch_assoc()) 
            { 
                $this->setTweetData('naam', $row['naam']); 
                $this->setTweetData('tweet', $row['tweet']); 
                $this->setTweetData('dag', $row['dag']); 
            } 
        } 
        $stmt->close; 
    } 


    private function setTweetData($key, $value) 
    { 
        $this->tweetData[$key] = $value; 
    } 

    private function getTweetData($key) 
    { 
        return $this->tweetData[$key]; 
    } 

    private function createTweets() 
    { 
        if ($this->ctweet == 5) 
        { 
            $style = "border-bottom: 1px solid #666666;"; 
        } 
            else 
        { 
            $style = ''; 
        } 

        if (strlen($this->getTweetData('tweet')) <50) 
        { 
            $top = '-28px;'; 
        } 
            else 
        { 
            $top = '-13px;'; 
        } 
        $this->content .=  '<div style="border-top: 1px solid #666666;  
                              padding: 8px 8px 0px 8px;  
                              width: 102%;  
                              margin-left: -10px;  
                              margin-right: -10px;  
                              overflow: hidden;  
                              position: relative;  
                              ' . $this->style . '  
                              background-color: #fff;"> 
                              <img style="position: absolute;  
                              top: ' . $this->top . '  
                              left: -3px;"  
                              src="http://www.habbo.nl/habbo-imaging/avatarimage?hb=img&user=' . $this->getTweetData("naam") . '&direction=3&gesture=sml" /> 
                            <div style="float: left;  
                            margin-left: 50px;  
                            width: 190px;"> 
                            <strong>' . $this->getTweetData("naam") . '</strong>: ' . $this->getTweetData("naam") . '</div> 
                            <div class="clear"  
                            style="clear: both;"></div> 
                            <div style="margin: 8px -8px 0px -8px;  
                            padding: 2px 8px 2px 2px;  
                            background-color: #FFFFFF;  
                            border-top: 1px solid #666666;  
                            position: relative;  
                            z-index: 2;  
                            text-align: right;  
                            color: #333333;  
                            font-size: 9px;">';         

        if (date("d-m-Y", strtotime($this->getTweetData('dag'))) == date('d-m-Y'))     
        { 
            $this->content .= 'Vandaag'; 
        }     
        elseif (date("d-m-Y", strtotime($this->getTweetData('dag'))) == date('d-m-Y', strtotime("-1 days")))  
        { 
            $this->content .= 'Gisteren'; 
        } 
        else  
        { 
            $this->content .= date('d-m-Y', strtotime($this->getTweetData("dag"))); 
        } 

        $this->content .= '</div></div>';         

        return $this->content; 

        $ctweet++;         
    }     

    public function showTweet() 
    { 
        $this->fetchTweets(); 
        echo $this->createTweets(); 

    } 
}
\$\endgroup\$
2
\$\begingroup\$

I'm not quite sure that I understand all parts of your code. It doesn't seem that you are ever printing the actual tweet ($row['tweet']), only the name of the person tweeting? And it also seems to me that you are fetching 5 semi-random tweets (as they are sorted by id, not date), but only using the data of the last one of those (setTweetData overrides the previously set data)?

$ctweet bug

$ctweet doesn't do anything right now. $ctweet++; stands after the return statements and is thus never executed.

Save value in temporary variable

This code:

date("d-m-Y", strtotime($this->getTweetData('dag')))

exists three times (which makes the code hard to read), and if it is an old tweet also gets executed three times (which is slow).

Just save it in a temporary variable.

Escape output

Right now, you are not escaping the data that you print ($this->getTweetData("naam")). You might want to escape it to prevent xss attacks.

Style Information

I would move the style information to an external CSS file. It is faster (because it can be cached separately from the HTML), and also leads to easier to read code.

\$\endgroup\$
2
\$\begingroup\$

Avoid SELECT * (with *) when possible

Make it a habit to specify explicitly the columns that you need, in your example:

SELECT naam, tweet, dag FROM tweet ORDER BY id DESC LIMIT 5

This way you can minimize the data transfer between your script and the database server, and your code will be more resilient to potential schema changes.

Bug?

What's the point of this?

while ($row = $result->fetch_assoc()) 
{ 
    $this->setTweetData('naam', $row['naam']); 
    $this->setTweetData('tweet', $row['tweet']); 
    $this->setTweetData('dag', $row['dag']); 
}

In each iteration you overwrite the values of naam, tweet, dag that were set by the previous iteration. Effectively you're saving only the 5th tweet, as the first 4 get overwritten.

Hardcoded style data

It's not good to hardcode style data like this:

$style = "border-bottom: 1px solid #666666;"; 
$top = '-28px;';

It would be best to use a template engine to separate your view from your logic. Or if that seems overkill, at least define your styles in a separate stylesheet file, and use CSS classes in the code.

\$\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.