1
\$\begingroup\$

I had made some pages and they work but I'm not sure if I coded it in best way so I want your suggestions and ideas to make my code better.

connection.php

    <?php
        $mysql_host = 'localhost';
        $mysql_user = 'root';
        $mysql_pass = 'root';
        $mysql_data = 'project_eye';

        $connect = mysql_connect($mysql_host, $mysql_user, $mysql_pass) or mysql_error();
        $db_sele = mysql_select_db($mysql_data);

        mysql_query("set names 'utf8'");
        mysql_query("SET character_set_client=utf8");
        mysql_query("SET character_set_connection=utf8");
        mysql_query("SET character_set_database=utf8");
        mysql_query("SET character_set_results=utf8");
        mysql_query("SET character_set_server=utf8");
    ?> 

phpCodes.php:
It's a page that contain the header. I'm using it to call the function to my page.

<?php
    function headerCode(){
        echo '
            <div class="header">
                <div class="header-top">
                    <div class="logform">
        ';
        accountLinks();
        echo '
            </div>
            <div class="social-newtork">
                        <a><img src="images/f.png"></a>
                        <a><img src="images/t.png"></a>
                        <a><img src="images/g.png"></a>
                    </div>          
                </div>
                <div class="menu-content">
                    <img src="images/eye.jpg">  
                    <img src="images/compelete.jpg" style="width: 78.3%">
                    <div class="desc">
                        <span class="first">عينٌـــــــــ</span>
                        <span class="second">على الحقيقة</span>
                    </div>
                </div>
                <div class="menu">
                    <ul>                
                        <li><a href="">محلية</a></li>                       
                        <li><a href="">عالمية</a></li>
                        <li><a href="">رياضية</a></li>                      
                        <li><a href="">طبية</a></li>
                        <li><a href="">طرائف</a></li>
                    </ul>
                </div>
            </div>
        ';
    }

    function accountLinks(){
            if( !isset($_SESSION['id']) ){
                echo '
                    <form method="post" action="index.php">                 
                        <input type="submit" name="submit" value="دخول" class="log">                            
                        <input type="password" id="password" name="password" placeholder="كلمة المرور" class="mem-information">
                        <input type="text" placeholder="اسم المستخدم" id="username" name="username" class="mem-information">
                    </form> ';
            }else{          
                echo '
                    <form method="post" action="index.php"> 
                        <a href="controlpanel/" class="account">لوحة التحكم</a>                 
                        <input type="submit" href="" class="logout" name="logout" onclick="logout(this);" value="تسجيل الخروح">
                    </form>
                ';
        }
    }


?>

con.php:
This page contains simple control panel and it didn't complete yet

<?php
    ob_start();
    session_start();
    include('../includes/connect.php');
    include('../includes/phpCodes.php');    
?>

<!DOCTYPE html>
<html>
    <head>
        <title>لوحة التحكم</title>
        <meta charset="utf-8">

        <link rel="stylesheet" type="text/css" href="../css/mainstyle.css">
        <link rel="stylesheet" type="text/css" href="css/controlstyle.css">
        <script type="text/javascript" src="http://ajax.googleapis.com/ajax/libs/jquery/1.3.0/jquery.min.js"></script>
        <script type="text/javascript">
            $(document).ready(function(){
                $('#tabs div').hide();
                $('#tabs div:first').show();
                $('#tabs ul li:first').addClass('active');
                $('#tabs ul li a').click(function(){ 
                    $('#tabs ul li').removeClass('active');
                    $(this).parent().addClass('active'); 
                    var currentTab = $(this).attr('href'); 
                    $('#tabs div').hide();
                    $(currentTab).show();
                    return false;
                });
            });
        </script>
    </head>
    <body>
    <div class="wrapper">
        <?php headerCode(); ?>
        <div class="content">
              <div id="tabs">
                <ul>
                    <li><a href="#add">اضافة موضوع</a></li>
                    <li><a href="#remove">حذف موضوع</a></li>
                    <li><a href="#edit">تعديل موضوع</a></li>
                    <li><a href="#edit">التحكم بالاقسام</a></li>
                </ul>
                <div id="add">
                    <form method="POST" action="includes/add.php" dir="rtl" enctype="multipart/form-data">
                        <br>

                        حدد القسم : <select name="section">
                            <?php
                                $query = "SELECT * FROM `sections`";
                                $result = mysql_query($query);
                                while($row=mysql_fetch_array($result, MYSQL_ASSOC)){                                                 
                                    echo "<option value='".$row['id']."'>".$row['sectionName']."</option>";
                                }
                            ?>
                        </select><br>
                        عنوان الموضوع :<input type="text" name="title" class="mem-information"/><br>
                        الموضوع : <br /><textarea name="subject" rows="10" cols="50" class="mem-information" style="width: 500px"></textarea><br /><br>
                        الصورة :<input type="file" name="image"><br>
                        <input type="submit" value="إرسال" name="send" class="log" style="color: black">
                    </form>
                </div>
                <div id="remove">
                    <form method="POST" action="includes/remove.php" dir="rtl"><br>
                    حدد القسم :
                    <select name ="sectionsName">
                    <option value="">dd</option>
                    </select>

                        <input type="submit" value="حذف" name="send" class="log" style="color: black">
                    </form>
                </div>
                <div id="edit">

                </div>
                <div id="addDep">

                </div>
            </div>
        </div>
    </div>
    </body>
</html> 

add.php:
It's job is to add values to database.

<?php
    session_start();
    include('../../includes/connect.php');

    $sectionID = $POST["section"];

    $title = $_POST['title'];
    $subject = $_POST['subject'];
    $visiable = 1;
    $imageName = mysql_real_escape_string($_FILES["image"]["name"]);
    $imageData = mysql_real_escape_string(file_get_contents($_FILES["image"]["tmp_name"]));
    $imageType = mysql_real_escape_string($_FILES["image"]["type"]);

    $query = "insert into news (title, subject, visiable, image, section_id) values ('$title','$subject', '$visiable', '$imageData', '$sectionID')"; 
    $result = mysql_query($query);
    $id = mysql_insert_id();

    $data = array(
            'id' => $id
            );
    $base = '../../show.php';
    $url = $base. '?' . http_build_query($data);
    header("Location: $url");
    exit();
?>

show.php:
After adding to database this page will display the values.

<?php
    ob_start();
    session_start();
    include('includes/connect.php');
    include('includes/phpCodes.php');


    function showNews(){
        $id = $_GET['id'];

         echo '<img src="includes/getImage.php?id=' . $id . '" class="newsImage">';


        echo '
            <h1><p class="subjecTitle">هنا العنوان</p></h1> 
                <div class="newsContent">
                    hihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihihi
                </div>
        ';
    }
?>

<!DOCTYPE html>
<html>
    <head>
        <title>عينٌ على الحقيقة</title>

        <meta charset="utf-8">

        <link rel="stylesheet" type="text/css" href="css/mainstyle.css">
        <link rel="stylesheet" type="text/css" href="css/showstyle.css">
        <script lang="javascript">
            function logout(myFrame){
                myFram.submit();
            }
        </script>
    </head>
    <body>
        <div class="wrapper">
            <?php headerCode(); ?>
            <div class="content" dir="rtl">
                <?php showNews(); ?>
            </div>
        </div>
    </body>
</html>
\$\endgroup\$
2
  • 5
    \$\begingroup\$ First of all start using mysqli_* or PDO functions to wrok with database. Mysql_* functions are depracted as of PHP 5.5 and will be removed soon \$\endgroup\$
    – DeiForm
    Commented Jul 30, 2013 at 8:49
  • 2
    \$\begingroup\$ Guiding you towards the "perfect" counterpart of this code will take some time, why don't you take yourself from the South-pole to the equater, so we can meet each other half way? because (respectfuly) this code is waaaay off. Oh and BTW: google: TMTOWTDI <= this means that "the perfect code" doesn't really exist, it all depends on what you need, and where \$\endgroup\$ Commented Jul 30, 2013 at 14:28

1 Answer 1

3
\$\begingroup\$

Ok, where to start.

mysql_* functions are bad. Burn them, throw them away but don't use them. Use mysqli_ instead or even better PDO.

Ever heard of SQL injections? I think so since you are using mysql_real_escape_string. A simple search on the interwebz for how to bypass mysql_real_escape_string shows its weaknesses.

That being said, let's look at your code.

The mysql_* left aside your code isn't bad at all. Everything works and at the end of the day that is all that matter. But after that day come other days. And in 2 month times you might need to change certain parts in your code, fix a bug, add some functionality,...

With that in our minds lets look at the code:

There is no real seperation of concern. Every file has multiple functionalities and knows way to much.

  • connection.php only handles the database connection. So this is good!
  • phpCodes.php supplies functions that handle presentation (I'll come to this later)
  • con.php handles presentation, sql queries, sessions and buffering
  • add.php knows about sessions AND how to add an Image.
  • show.php knows about session, buffers, other functional files AND it knows about presentation. Now that is a lot for a simple file.

I don't think I need to tell you what is wrong ;)

One of the important programming rules (imo) is DRY, Don't repeat yourself. Yet somehow you repeat the HTML in 2 files. So if you want to make a change to the html you will have to check all files that have html and change it there. So make sure that you don't write duplicate code. Not for 'increased performace' because of a smaller filesize but because it is way easier to maintain.

So, a little todo list for you: Seperate business logic from presentation. In your presentation files there sould only be html with some echo $someVar statements. In your business logic there should be no html and plain stupid code. Code that does the job without having to know about other parts of the system.

Then create a 'controller' file that includes the correct files, starts the session and the buffer, ...

This way you can easily add functionality, change presentation, ...

Always think SOLID

One last remark on the phpCodes.php. Don't use functions to output html. Functions should give some sort of functionality. You give it something and it does something to that thing. But you are simply using it as a glorified variable holding some HTML.

A part from that a function should best be stateless. But your function returns different things depending on a variable that doesn't even get passed in.

If a function changes its output depending on a variable, that variable should be passed in into the function. Not hard coded in it. If the sole purpose of the function is to return html. Don't use a function, simply include the correct template in the controller.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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