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.

I made this simple chat in php. You have suggestions on how I can improve it?

This file configures the connection to the server

This file is a simple form "login" ( only username required )

 <form action="chat.php" method = "post">
    Username: <input type="text" name="nick"/><br />
    <input type="submit" name="button" value="Go"/>
    </form>
    <?php

    include('connessione.php');
    if(isset($_POST['button'])) {
        $Username = $_POST['nick'];
        $Username = mysql_real_escape_string($Username);
        $Query = "INSERT INTO users (NickName) VALUES ($Username)";
        if(!$Query) {
        }
        else {
            "Error performing query!".mysql_error();
        }
    }
    ?>

This file is the chat!

<script type="text/javascript" src="chat.js"></script>
    <style type="text/css">
    .tastiera {
        width:500px;
        height:50px;
        border-radius:5px;
        border:1px solid #999;
        overflow:auto;
        font-size:13px;

    .chat {
        width:500px;
        height:6000px;
        border-radius:5px;
        border:1px solid #999;
        overflow:auto;
        font-size:13px;
    }

    </style>
    <?php
            include('connessione.php');
            session_start ();
            if($_SESSION['nick'] == “”){
                echo "You are not authorized to enter!";
            }
            exit();
            header('Cache-Control: Private');



    ?>
    <div id="CHAT"></div>
    <form action="salvataggio.php" name="inserimento" method="post" onsubmit=”javascript:location.reload();”>
    <input type="text" name="messaggio" width="500" height="50"/>
    <input type="submit" value="Chat"/>

    </form>
<iframe src="messaggio.php" name="MSG" id="MSG"></iframe>

This Ajax file Update the chat

   function Update()
    {
      return Request();
    }
    window.setInterval("Update()", 3000)
    var XMLHTTP;
    function Request()
    {
      XMLHTTP = GetBrowser(ChangeStatus);
      XMLHTTP.open("GET", "ajax.php", true);
      XMLHTTP.send(null);
    }
    function ChangeStatus()
    {
      if (XMLHTTP.readyState == 4)
      {
        var R = document.getElementById("CHAT");
        R.innerHTML = XMLHTTP.responseText;
      }
    }
    function GetBrowser(FindBrowser)
    {
      if (navigator.userAgent.indexOf("MSIE") != (-1))
      {
        var Class = "Msxml2.XMLHTTP";
        if (navigator.appVersion.indexOf("MSIE 5.5") != (-1));
        {
          Class = "Microsoft.XMLHTTP";
        } 
        try
        {
          ObjXMLHTTP = new ActiveXObject(Class);
          ObjXMLHTTP.onreadystatechange = FindBrowser;
          return ObjXMLHTTP;
        }
        catch(e)
        {
          alert("attenzione: l'ActiveX non sarà eseguito!");
        }
      }
      else if (navigator.userAgent.indexOf("Mozilla") != (-1))
      {
        ObjXMLHTTP = new XMLHttpRequest();
        ObjXMLHTTP.onload = FindBrowser;
        ObjXMLHTTP.onerror = FindBrowser;
        return ObjXMLHTTP;
      }
      else
      {
        alert("L'esempio non funziona con altri browser!");
      }
    }

This file save into database the messages

<?php
@session_start();
if(!isset($_SESSION['nick'])){
  @header('Location:prechat.php');
}else{
  if(isset($_POST['messaggio'])){
    include 'connessione.php';
    $user = $_SESSION['nick'];
    $mex_chat = addslashes($_POST['messaggio']);
    $query = "INSERT INTO utenti (user, mex_chat) 
    VALUES ('$user','$mex_chat')";
    @mysql_query($query)or die (mysql_error());
    @mysql_close();
    @header('Location:chat.php');
  }
}

This file display the messages

$sql = "SELECT user, mex_chat FROM 
utenti ORDER BY id_chat DESC LIMIT 0,10";
$sql_res = @mysql_query($sql)or die (mysql_error());
if(@mysql_num_rows($sql_res)>0)
{
  while ($fetch = @mysql_fetch_array($sql_res))
  { 
    $utente = stripslashes($fetch['user_chat']);
    $mex_utente = stripslashes($fetch['mex_chat']);
    echo '<b>'. $utente .'</b>: '. $mex_utente.'<br />';
  }
}else{
  echo 'Non sono stati ancora inseriti dei messaggi.';
}
?>

What do you think? How can I improve or resolve any bugs ( if there are ) ?

share|improve this question
If you aren't sure if this is working 100%, then come back when you have fixed it. – Joseph the Dreamer Aug 24 at 17:23

migrated from stackoverflow.com Aug 24 at 16:36

This question came from our site for professional and enthusiast programmers.

1 Answer

If I'm reading this right, your AJAX grabs the whole chat every 3 seconds and (re)displays it. IMO, it'd be much better to send some info on the last message you have (timestamp or something), and then fetch only newer messages if there are any (via JSON; format them on the client side). That would reduce the load on the network which, given the short update interval, might be significant for both your and your users' bandwidth.

I suggest posting messages via AJAX as well, so that the page never gets reloaded. It's more pleasant for the user (and requires less network load, if done properly).

And, of course, lose the mysql_* functions! These are insecure and depricated, and are to be abandoned soon. I prefer PDO, but you can also go for mysqli_* functions. Your current code is wide open for attacks.

share|improve this answer

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.