Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I would like to improve the following code that I posted below. It does the job perfectly, but I just feel like it could definitely be improved. I could easily leave it the way it is but I am trying to learn and that's why I would like to get suggestions from experienced programmers.

Code:

<?php
if($facebook && !$twitter_username || !$facebook && $twitter_username) {
    $width = "490";
} else {
    $width = "930";
}
?>

<div id="content">
<div style="width:<?=$width;?>px;margin:auto;">
    <?php if($facebook) { ?><div class="buttons"><img src="images/facebook_icon.png" class="fb" /> <p><em>Like Us</em><br />on Facebook</p></div><?php } ?>
    <?php if($twitter_username) { ?><div class="buttons"><img src="images/twitter_icon.png" class="twitter" /> <p><em>Follow Us</em><br />on Twitter</p><span>@<?=$twitter_username;?></span></div><?php } ?>
</div>
<br class="clear" />

<?php 
if($twitter_username) {
    if($display_tweets) {
 ?>
<div class="tweet">
    <h2 class="twitter_feed white_textshadow">Latest Tweet</h2>
    <div class="feed"></div>
</div>
<?php 
    }

} 
if(!$display_tweets || !$twitter_username) {
    if($website_url) { 
?>
    <h2 class="visit white_textshadow center"><em>Visit Our Website</em><br>www.<?=$website_url;?></h2>
<?php 
    }
}
?>
</div>

This is for a page where it displays some Social Media information for that user. In the database there are 4 fields:

  • facebook (NULL or 1)
  • twitter username (NULL or username)
  • display_feed (NULL or 1)
  • website URL (NULL or url domain.com format)

Any tips on how to improve this, I would really appreciate it. Maybe creating some functions so I don't have to keep having these if and else statements.

Thank you!

share|improve this question
1  
Consider using a templating engine like twig: twig.sensiolabs.org – igorw Dec 31 '11 at 12:28

1 Answer

up vote 4 down vote accepted

Style (PHP)

  • At the beginning, the test seems pretty weird but in any case, I usually prefer 

    $var = (condition) ? val1 : val2;

    over 

    if(condition) {
    $var = val1;
} else {
    $var = val2;
}

  • You can replace

    if(condition1) {
    if(condition2) {
        stuff();
    }
}

    with

    if(condition1 && condition2) {
    stuff();
}

On top of that, the if(!$display_tweets || !$twitter_username) test would then be useless as it would correspond to the else case.

  • There's no point in closing php tags just before reopening them. Remove the ? > < ? php tags.

Style (HTML)

  • Do you really want to use the em markup inside the h2 one?

What you are actually doing/trying to do

  • The code at the beginning seems to be assuming that we have a least facebook or twitter (sometimes both).

    • If this is indeed the case, the $width = ($facebook && !$twitter_username || !$facebook && $twitter_username) ? "490" : "930"; at the beginning could become $width = ($facebook && $twitter_username) ? "930" : "490";
    • If it is not the case, could the beginning be behind an if (twitter || facebook) test as creating an empty div (and giving it an arbitrary width) might not be relevant. If you want to keep it, for the definition of the width, I'd rather have someting like : $width = ($facebook && $twitter_username) ? "930" : (($facebook || $twitter_username) ? "490" : "1");

  • It might be interesting to use <a href="<?= $website_url; ?>" > or something like that to display the url as an actual link to make things easier from a user point of view (here I assumed that the input has been sanitized properly). On top of that, I don't understand why you hardcode the www. string (have a look at your current adress bar for more details). If there was anything to be hardcoded (and I personally wouldn't go for that), it's more likely to be the http:// part.

share|improve this answer
Thanks so much for your help. For the em inside the h2, I needed that part to be italics and then also that italicized part has a little bigger font size then the bottom part, so I can target it with CSS to increase the font size. Is there a better way to do it? And for the www, I am storing the domains as domain.com in the database and this screen will be shown on TV's so there are no clickable links or anything. I just figured it would be best to display the website URL as www.domain.com - should I do it differently? – Drew Dec 30 '11 at 21:27
I would use a <span> or a <p> tag in the place of the em. Then apply a class to the new element rather than using em. – jezzipin Feb 7 at 10:07

Your Answer

 
discard

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.