Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I would love to have your reviews of this sign in and sign up system.

You can see the main structure:

enter image description here

For now, I have a total of 13 files (index.php, top_bar.php, header.php, container.php, footer.php, sign_in.php, sign_up.php, members.php, help.php, contact_us.php, change_theme.php, rules.php and the style.css.


This is how my main page functions:

index.php:

<!DOCTYPE html>
<html lang="en-us">
    <head>
        <meta charset="UTF-8">
        <?php
        if(strpos($_SERVER["SCRIPT_FILENAME"], "index") !== false) { ?>
        <title>Test - Forums</title>
        <?php } ?>
        <?php
        if(strpos($_SERVER["SCRIPT_FILENAME"], "members") !== false) { ?>
        <title>Test - Members</title>
        <?php } ?>
        <?php
        if(strpos($_SERVER["SCRIPT_FILENAME"], "sign_up") !== false) { ?>
        <title>Test - Sign Up</title>
        <?php } ?>
        <?php
        if(strpos($_SERVER["SCRIPT_FILENAME"], "sign_in") !== false) { ?>
        <title>Test - Sign In</title>
        <?php } ?>
        <?php
        if(strpos($_SERVER["SCRIPT_FILENAME"], "change_theme") !== false) { ?>
        <title>Test - Change Theme</title>
        <?php } ?>
        <?php
        if(strpos($_SERVER["SCRIPT_FILENAME"], "contact_us") !== false) { ?>
        <title>Test - Contact Us</title>
        <?php } ?>
        <?php
        if(strpos($_SERVER["SCRIPT_FILENAME"], "help") !== false) { ?>
        <title>Test - Help</title>
        <?php } ?>
        <?php
        if(strpos($_SERVER["SCRIPT_FILENAME"], "rules") !== false) { ?>
        <title>Test - Rules</title>
        <?php } ?>
        <link href="css/style.css" rel="stylesheet" type="text/css">
        <link href="//maxcdn.bootstrapcdn.com/font-awesome/4.1.0/css/font-awesome.min.css" rel="stylesheet" type="text/css">
    </head>
    <body>
        <?php include("top_bar.php");?>
        <?php include("header.php");?>
        <?php include("container.php");?>
        <?php include("footer.php");?>
    </body>
</html>

This is how my main pages (top_bar.php, header.php, container.php and footer.php) are structured:

top_bar.php:

<!-- TOP BAR -->
<div id="top_bar">
    <div class="wrapper">
        <div id="top_bar_links">
            <ul>
            <?php
            $full_name = $_SERVER["PHP_SELF"];
            $name_array = explode("/",$full_name);
            $count = count($name_array);
            $page_name = $name_array[$count-1];
            ?>
            <li id="home">
                <a href="../">Home</a>
            </li>
            <li>
                <a class="<?php echo ($page_name=="index.php")?"active":"";?>" href=".">Forums</a>
            </li>
            <li>
                <a class="<?php echo ($page_name=="members.php")?"active":"";?>" href="members.php">Members</a>
            </li>
            </ul>
        </div>
    </div>
</div>

header.php:

<!-- HEADER -->
<div id="header">
    <div class="wrapper">
        <h1 id="logo">
            <a href=".">Test</a>
        </h1>
        <div id="member_links">
            <ul>
                <li id="sign_up">
                    <a href="sign_up.php">Sign Up</a>
                </li>
                <li id="sign_in">
                    <a href="sign_in.php">Sign In</a>
                </li>
            </ul>
        </div>
    </div>
</div>

container.php:

<!-- CONTAINER -->
<div class="wrapper">
    <div id="container">
        <div id="breadcrumb_top">
            <div class="breadcrumb_links">
                <ul>
                </ul>
            </div>
        </div>
        <?php
        if(strpos($_SERVER["SCRIPT_FILENAME"], "index") !== false) { ?>
        <h1 style="margin-bottom: 15px;">Forums</h1>
        <h3 id="category_title">Categories</h3>
        <?php } ?>
        <?php
        if(strpos($_SERVER["SCRIPT_FILENAME"], "members") !== false) { ?>
        <h1 style="margin-bottom: 15px;">Members</h1>
        <?php } ?>
        <?php
        if(strpos($_SERVER["SCRIPT_FILENAME"], "sign_up") !== false) { ?>
        <h1 style="text-align: center; margin-bottom: 15px;">Sign Up</h1>
        <?php } ?>
        <?php
        if(strpos($_SERVER["SCRIPT_FILENAME"], "sign_in") !== false) { ?>
        <h1 style="text-align: center; margin-bottom: 15px;">Sign In</h1>
        <?php } ?>
        <?php
        if(strpos($_SERVER["SCRIPT_FILENAME"], "change_theme") !== false) { ?>
        <h1 style="margin-bottom: 15px;">Change Theme</h1>
        <?php } ?>
        <?php
        if(strpos($_SERVER["SCRIPT_FILENAME"], "contact_us") !== false) { ?>
        <h1 style="margin-bottom: 15px;">Contact Us</h1>
        <?php } ?>
        <?php
        if(strpos($_SERVER["SCRIPT_FILENAME"], "help") !== false) { ?>
        <h1 style="margin-bottom: 15px;">Help</h1>
        <?php } ?>
        <?php if(strpos($_SERVER["SCRIPT_FILENAME"], "rules") !== false) { ?>
        <h1 style="margin-bottom: 10px;">Rules</h1>
        <p style="margin-bottom: 15px;">Lorem ipsum dolor sit amet, consectetur adipiscing elit. Proin eu nibh turpis. Nunc sit amet auctor elit. Lorem ipsum dolor sit amet, consectetur adipiscing elit. In malesuada lobortis tempus. Integer auctor condimentum sapien, non scelerisque eros cursus et. In vel leo elementum, iaculis tellus sit amet, vestibulum quam. Etiam dapibus pulvinar risus, vestibulum rhoncus sapien commodo vitae. Etiam sit amet ultrices dui. Suspendisse luctus fringilla eros. Nam vitae metus porttitor, sagittis arcu eleifend, malesuada odio. Aliquam erat volutpat.</p>
        <p style="margin-bottom: 15px;">Pellentesque id velit a elit porttitor sollicitudin et vulputate nisl. Donec eu purus non libero porta malesuada et non lorem. Vestibulum ultrices vitae elit vitae accumsan. Quisque euismod, quam sed ornare ultrices, magna mi posuere massa, vel placerat ipsum est quis erat. Aliquam non libero mauris. Etiam ligula velit, commodo et feugiat ac, porta eu orci. Donec laoreet ipsum in urna auctor, vitae malesuada nibh consequat. Donec sit amet libero vitae erat rhoncus venenatis. Maecenas nec pretium justo, eget fermentum tellus. Ut aliquet tellus venenatis posuere fermentum. Fusce mattis velit et tellus suscipit consectetur.</p>
        <?php } ?>
        <div id="breadcrumb_bottom">
            <div class="breadcrumb_links">
                <ul>
                </ul>
            </div>
        </div>
    </div>
</div>

footer.php:

<!-- FOOTER -->
<div class="wrapper">
    <div id="footer">
        <div id="footer_links">
            <ul>
                <li id="change_theme">
                    <a href="change_theme.php">Change Theme</a>
                </li>
                <li id="contact_us">
                    <a href="contact_us.php">Contact Us</a>
                </li>
                <li id="help">
                    <a href="help.php">Help</a>
                </li>
                <li id="rules">
                    <a href="rules.php">Rules</a>
                </li>
            </ul>
        </div>
        <p id="footer_copyright">Forum software coded by Dylan - 2014</p>
    </div>
</div>

What do you think? Is it good?

I would like your help, as I am more knowledgable in HTML and CSS. I don't have much knowledge in PHP.

share|improve this question

2 Answers 2

There's a few things wrong with this. Primarily that there is no separation of concerns - you have PHP mixed with HTML, etc. However, barring that, there are some low hanging fruit that we can nip:

Your index.php contains unnecessary mixtures of <?php if(){ ?>...<?}?> etc. With a switch statement, the index.php can become more concise, while retaining the same functionality. Additionally, there's not need for multiple in the includes. Just one will do:

<!DOCTYPE html>
<html lang="en-us">
    <head>
        <meta charset="UTF-8">
        <title>Test - <?php
        switch(trim($_SERVER['SCRIPT_FILENAME'])){
            case "members":         echo "Members";         break;
            case "sign_up":         echo "Sign Up";         break;
            case "sign_in":         echo "Sign In";         break;
            case "change_theme":    echo "Change Theme";    break;
            case "contact_us":      echo "Contact Us";      break;
            case "help":            echo "Help";            break;
            case "rules":           echo "Rules";           break;
            default:                echo "Forums";          break;
        }?></title>

        <link href="css/style.css" rel="stylesheet" type="text/css">
        <link href="//maxcdn.bootstrapcdn.com/font-awesome/4.1.0/css/font-awesome.min.css" rel="stylesheet" type="text/css">
    </head>
    <body>
        <?php 
          include("top_bar.php");
          include("header.php");
          include("container.php");
          include("footer.php");
        ?>
    </body>
</html>

However, seeing as you're not doing any logic on the includes - why not just have it as one page?

For top_bar.php, you can get rid of most of the php code by using this more concise version:

<!-- TOP BAR -->
<div id="top_bar">
    <div class="wrapper">
        <div id="top_bar_links">
            <ul>
            <?php
            $page_name = array_pop(explode("/", $_SERVER["PHP_SELF"]));
            ?>
            <li id="home">
                <a href="../">Home</a>
            </li>
            <li>
                <a class="<?php echo ($page_name=="index.php")?"active":"";?>" href=".">Forums</a>
            </li>
            <li>
                <a class="<?php echo ($page_name=="members.php")?"active":"";?>" href="members.php">Members</a>
            </li>
            </ul>
        </div>
    </div>
</div>

Again, in container.php as in index.php, a switch statement benefits you. Also, abstracting margin:15px to a CSS class is cleaner:

<style>
    .h1_marg_b15{
        margin-bottom: 15px;
    }
</style>

<!-- CONTAINER -->
<div class="wrapper">
    <div id="container">
        <div id="breadcrumb_top">
            <div class="breadcrumb_links">
                <ul>
                </ul>
            </div>
        </div>
        <?php
        switch(trim($_SERVER['SCRIPT_FILENAME']){
            case 'index': {
                echo '<h1 class="h1_marg_b15">Forums</h1>';
                echo '<h3 id="category_title">Categories</h3>';
            } break;
            case 'members': {
                echo '<h1 class="h1_marg_b15">Members</h1>';
            } break;
            case 'sign_up': {
                echo '<h1 class="h1_marg_b15" style="text-align: center;">Sign Up</h1>';
            } break;
            case 'sign_in': {
                echo '<h1 class="h1_marg_b15" style="text-align: center;">Sign In</h1>';
            } break;
            case 'change_theme': {
                echo '<h1 class="h1_marg_b15">Change Theme</h1>';
            } break;
            case 'contact_us': {
                echo '<h1 class="h1_marg_b15">Contact Us</h1>';
            } break;
            case 'help': {
                echo '<h1 class="h1_marg_b15">Help</h1>';
            } break;
            case 'rules': {
                echo '<h1 style="margin-bottom: 10px;">Rules</h1>';
                echo '<p style="margin-bottom: 15px;">Lorem ipsum dolor sit amet, consectetur adipiscing elit. Proin eu nibh turpis. Nunc sit amet auctor elit. Lorem ipsum dolor sit amet, consectetur adipiscing elit. In malesuada lobortis tempus. Integer auctor condimentum sapien, non scelerisque eros cursus et. In vel leo elementum, iaculis tellus sit amet, vestibulum quam. Etiam dapibus pulvinar risus, vestibulum rhoncus sapien commodo vitae. Etiam sit amet ultrices dui. Suspendisse luctus fringilla eros. Nam vitae metus porttitor, sagittis arcu eleifend, malesuada odio. Aliquam erat volutpat.</p>';
                echo '<p style="margin-bottom: 15px;">Pellentesque id velit a elit porttitor sollicitudin et vulputate nisl. Donec eu purus non libero porta malesuada et non lorem. Vestibulum ultrices vitae elit vitae accumsan. Quisque euismod, quam sed ornare ultrices, magna mi posuere massa, vel placerat ipsum est quis erat. Aliquam non libero mauris. Etiam ligula velit, commodo et feugiat ac, porta eu orci. Donec laoreet ipsum in urna auctor, vitae malesuada nibh consequat. Donec sit amet libero vitae erat rhoncus venenatis. Maecenas nec pretium justo, eget fermentum tellus. Ut aliquet tellus venenatis posuere fermentum. Fusce mattis velit et tellus suscipit consectetur.</p>';
            } break;
        }
        ?>
        <div id="breadcrumb_bottom">
            <div class="breadcrumb_links">
                <ul>
                </ul>
            </div>
        </div>
    </div>
</div>

There are a handful of other things that are of concern (not checking for empty values, etc), but they are inconsequential in this scope.

Putting it all together, into one page, would look something like:

<?php
# Select the title
switch(trim($_SERVER['SCRIPT_FILENAME'])){
    case "members":         $title = "Members";         break;
    case "sign_up":         $title = "Sign Up";         break;
    case "sign_in":         $title = "Sign In";         break;
    case "change_theme":    $title = "Change Theme";    break;
    case "contact_us":      $title = "Contact Us";      break;
    case "help":            $title = "Help";            break;
    case "rules":           $title = "Rules";           break;
    default:                $title = "Forums";          break;
}

# Define the page name and check for empty value
$page_name = array_pop(explode("/", $_SERVER["PHP_SELF"])); 
if(empty($page_name)) $page_name = "index.php";

# Select the sidecase
$sidecase = "";
switch(trim($_SERVER['SCRIPT_FILENAME']){
    case 'index': {
        $sidecase .= '<h1 class="h1_marg_b15">Forums</h1>';
        $sidecase .= '<h3 id="category_title">Categories</h3>';
    } break;
    case 'members': {
        $sidecase .= '<h1 class="h1_marg_b15">Members</h1>';
    } break;
    case 'sign_up': {
        $sidecase .= '<h1 class="h1_marg_b15" style="text-align: center;">Sign Up</h1>';
    } break;
    case 'sign_in': {
        $sidecase .= '<h1 class="h1_marg_b15" style="text-align: center;">Sign In</h1>';
    } break;
    case 'change_theme': {
        $sidecase .= '<h1 class="h1_marg_b15">Change Theme</h1>';
    } break;
    case 'contact_us': {
        $sidecase .= '<h1 class="h1_marg_b15">Contact Us</h1>';
    } break;
    case 'help': {
        $sidecase .= '<h1 class="h1_marg_b15">Help</h1>';
    } break;
    case 'rules': {
        $sidecase .= '<h1 style="margin-bottom: 10px;">Rules</h1>';
        $sidecase .= '<p style="margin-bottom: 15px;">Lorem ipsum dolor sit amet, consectetur adipiscing elit. Proin eu nibh turpis. Nunc sit amet auctor elit. Lorem ipsum dolor sit amet, consectetur adipiscing elit. In malesuada lobortis tempus. Integer auctor condimentum sapien, non scelerisque eros cursus et. In vel leo elementum, iaculis tellus sit amet, vestibulum quam. Etiam dapibus pulvinar risus, vestibulum rhoncus sapien commodo vitae. Etiam sit amet ultrices dui. Suspendisse luctus fringilla eros. Nam vitae metus porttitor, sagittis arcu eleifend, malesuada odio. Aliquam erat volutpat.</p>';
        $sidecase .= '<p style="margin-bottom: 15px;">Pellentesque id velit a elit porttitor sollicitudin et vulputate nisl. Donec eu purus non libero porta malesuada et non lorem. Vestibulum ultrices vitae elit vitae accumsan. Quisque euismod, quam sed ornare ultrices, magna mi posuere massa, vel placerat ipsum est quis erat. Aliquam non libero mauris. Etiam ligula velit, commodo et feugiat ac, porta eu orci. Donec laoreet ipsum in urna auctor, vitae malesuada nibh consequat. Donec sit amet libero vitae erat rhoncus venenatis. Maecenas nec pretium justo, eget fermentum tellus. Ut aliquet tellus venenatis posuere fermentum. Fusce mattis velit et tellus suscipit consectetur.</p>';
    } break;
}
?>

<!DOCTYPE html>
<html lang="en-us">
    <head>
        <meta charset="UTF-8">
        <title>Test - <?php echo $title; ?></title>

        <link href="css/style.css" rel="stylesheet" type="text/css">
        <link href="//maxcdn.bootstrapcdn.com/font-awesome/4.1.0/css/font-awesome.min.css" rel="stylesheet" type="text/css">

        <style>
            .h1_marg_b15{
                margin-bottom: 15px;
            }
        </style>
    </head>
    <body>
        <!-- TOP BAR -->
        <div id="top_bar">
            <div class="wrapper">
                <div id="top_bar_links">
                    <ul>
                    <li id="home">
                        <a href="../">Home</a>
                    </li>
                    <li>
                        <a class="<?php echo ($page_name=="index.php")?"active":"";?>" href=".">Forums</a>
                    </li>
                    <li>
                        <a class="<?php echo ($page_name=="members.php")?"active":"";?>" href="members.php">Members</a>
                    </li>
                    </ul>
                </div>
            </div>
        </div>

        <!-- HEADER -->
        <div id="header">
            <div class="wrapper">
                <h1 id="logo">
                    <a href=".">Test</a>
                </h1>
                <div id="member_links">
                    <ul>
                        <li id="sign_up">
                            <a href="sign_up.php">Sign Up</a>
                        </li>
                        <li id="sign_in">
                            <a href="sign_in.php">Sign In</a>
                        </li>
                    </ul>
                </div>
            </div>
        </div>

        <!-- CONTAINER -->
        <div class="wrapper">
            <div id="container">
                <div id="breadcrumb_top">
                    <div class="breadcrumb_links">
                        <ul>
                        </ul>
                    </div>
                </div>
                <?php echo $sidecase; ?>
                <div id="breadcrumb_bottom">
                    <div class="breadcrumb_links">
                        <ul>
                        </ul>
                    </div>
                </div>
            </div>
        </div>

        <!-- FOOTER -->
        <div class="wrapper">
            <div id="footer">
                <div id="footer_links">
                    <ul>
                        <li id="change_theme">
                            <a href="change_theme.php">Change Theme</a>
                        </li>
                        <li id="contact_us">
                            <a href="contact_us.php">Contact Us</a>
                        </li>
                        <li id="help">
                            <a href="help.php">Help</a>
                        </li>
                        <li id="rules">
                            <a href="rules.php">Rules</a>
                        </li>
                    </ul>
                </div>
                <p id="footer_copyright">Forum software coded by Dylan - 2014</p>
            </div>
        </div>
    </body>
</html>
share|improve this answer
1  
Code Review is not the place for tutorials. Comments removed. –  rolfl Jul 30 at 21:21
    
With my code, I know how to do this, but this is in relation to the code that you gave me. I would like to use it, but I do not know how. Can you give me a statement? I do not know how to put a title for each of my pages with your code and I know not why there is "sidecase" everywhere. An explanation, please. –  We7dy Jul 30 at 21:25

I was writing an answer but @jsanc623 beat me to the punch with the switch, so I'll just add the missing bit.

Your SCRIPT_FILENAME solution won't work if your file is served from e.g. /var/www/my-site/members/forum/index.php.

Instead of doing it this way, you can find the current filename more reliably like so:

$basename = basename($_SERVER['SCRIPT_FILENAME']); // => index.php

Or, to get just the file name:

$filename = basename($_SERVER['SCRIPT_FILENAME'], '.php'); // => index
// or more generally:
$filename = path_info($_SERVER['SCRIPT_FILENAME'], PATHINFO_FILENAME); // => index

However, I'm curious as to why that logic is in there at all; it won't ever evaluate to anything but "index" since it won't be called from e.g. members.php...

share|improve this answer
    
Good catch on $_SERVER never evaluating to anything but "index" –  jsanc623 Jul 30 at 19:29
1  
@Schism Could you not just use basename()? I believe they return the same thing, plus it's a little easier to read. –  Alex L Jul 30 at 19:59
1  
@AlexL You totally could. I originally had path_info because I wanted to use PATHINFO_FILENAME, but I see now that basename is capable of that too. –  Schism Jul 30 at 20:12

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.