Page 1 of 1

First attempt of using switch

Posted: Sun Oct 28, 2007 1:34 am
by shivam0101

Code: Select all

<?php
   ob_start();
   error_reporting(E_ALL);
   require_once('config/includes.php');
   
   $process=isset($_GET['process']) ? $_GET['process']: '';
   $member_username=isset($_POST['member_username']) ? $_POST['member_username'] : '';
   $member_password=isset($_POST['member_password']) ? $_POST['member_password'] : ''; 
   
   switch($process)
   {
    case 'submit':
          submit($member_username, $member_password);
          break;
    
   
    default:
         show_form($member_username, $member_password, '');
         break; 
   }
   

  function submit($member_username, $member_password)
  {
    $msgs=validate($member_username, $member_password);
    if(count($msgs) >0)
    {
       $message="";
       foreach($msgs as $messages)
       {
           $message.=$messages.'<br/>';
       }
       show_form($member_username, $member_password, $message);
    }
    else
    {
      $query_check=mysql_query("SELECT * FROM members WHERE member_username=". "'$member_username'" . " AND member_password=". "'$member_password'");
      if(mysql_num_rows($query_check) > 0)
      {
            $fetch_member_det=mysql_fetch_array($query_check);
            $member_id=$fetch_member_det['member_id'];
            session_start();
            $_SESSION['member_id']=$member_id;
            header("Location: ".SITE_URL."members.php");
      }
      else
      {
            $message="Invalid login";
            show_form($member_username, $member_password, $message);
      }
    } 
  }
  
  function validate($member_username, $member_password)
  {
    $mistake=array();
    if(empty($member_username))     array_push($mistake, 'Username is empty');
    if(empty($member_password))     array_push($mistake, 'Password is empty');
    
    return $mistake;
  }    
     
   
   function show_form($member_username, $member_password, $message)
   {
     $member_username=stripslashes($member_username);
     $member_password=stripslashes($member_password);
     
     $login_table=
     "<form method='post' action='index.php?process=submit'>
      <table border='1' align='center'>
        <tr>
         <td>Username</td>
         <td><input type='text' name='member_username' value=$member_username></td>
        </tr>
        <tr>
         <td>Password</td>
         <td><input type='password' name='member_password' value='$member_password'></td>
        </tr>
        <tr>
         <td colspan='2' align='center'><input type='submit' name='submit' value='submit'></td>
        </tr>
     </table>
     </form>";
     
     $data=array('{heading}'=>'Login', '{content}' => $login_table, '{message}' => $message, '{links}' => '&nbsp;');
     ReadTemplate('templates/main_temp.html', $data);
  }
  
ob_end_flush();
?>

Posted: Sun Oct 28, 2007 3:49 am
by Kieran Huggins
isset() returns TRUE or FALSE, not the value of the variable it checks

Posted: Sun Oct 28, 2007 4:42 am
by Benjamin
That's fine, he's using a ternary operator.

Posted: Sun Oct 28, 2007 5:01 am
by Kieran Huggins
oops :oops: totally missed that paren.

/ignore

Posted: Sun Oct 28, 2007 8:20 am
by feyd
shivam0101, you haven't explained anything. Please do so.

Posted: Sun Oct 28, 2007 8:44 am
by shivam0101
update code//

Code: Select all

<?php
   ob_start();
   error_reporting(E_ALL);
   require_once('config/includes.php');
   
   $process=isset($_GET['process']) ? $_GET['process']: '';
   $member_username=isset($_POST['member_username']) ? $_POST['member_username'] : '';
   $member_password=isset($_POST['member_password']) ? $_POST['member_password'] : ''; 
   
   switch($process)
   {
    case 'submit':
          submit($member_username, $member_password);
          break;
    
   
    default:
         show_form($member_username, $member_password, '');
         break; 
   }
   

  function submit($member_username, $member_password)
  {
    $msgs=validate($member_username, $member_password);
    if(count($msgs) >0)
    {
       $message="Please check the following errors <br/>";
       foreach($msgs as $messages)
       {
           $message.=$messages.'<br/>';
       }
       show_form($member_username, $member_password, $message);
    }
    else
    {
      $query_check=mysql_query("SELECT * FROM members WHERE member_username=". "'$member_username'" . " AND member_password=". "'$member_password'");
      if(mysql_num_rows($query_check) > 0)
      {
            $fetch_member_det=mysql_fetch_array($query_check);
            $member_id=$fetch_member_det['member_id'];
            session_start();
            
            $_SESSION['member_id']=$member_id;
            if(defined('SITE_URL'))
             header("Location: ".SITE_URL."members.php");
      }
      else
      {
            $message="Invalid login";
            show_form($member_username, $member_password, $message);
      }
    } 
  }
  
  function validate($member_username, $member_password)
  {
    $mistake=array();
    if(empty($member_username))     array_push($mistake, 'Username is empty');
    if(empty($member_password))     array_push($mistake, 'Password is empty');
    
    return $mistake;
  }    
     
   
   function show_form($member_username, $member_password, $message)
   {
     $login_table=
     "<form method='post' action='index.php?process=submit'>
      <table border='1' align='center'>
        <tr>
         <td>Username</td>
         <td><input type='text' name='member_username' value=$member_username></td>
        </tr>
        <tr>
         <td>Password</td>
         <td><input type='password' name='member_password' value='$member_password'></td>
        </tr>
        <tr>
         <td colspan='2' align='center'><input type='submit' name='submit' value='submit'></td>
        </tr>
     </table>
     </form>";
     
     $data=array('{heading}'=>'Login', '{content}' => $login_table, '{message}' => $message, '{links}' => '&nbsp;');
     ReadTemplate('templates/main_temp.html', $data);
  }
  
ob_end_flush();
?>
This is the login page. The same code (pattern) will be used throught the site. Should i have to do more validation (like length of the string, is it array or string etc?). If there are any security issues or anything which can be done in a better way, please let me know

Posted: Sun Oct 28, 2007 9:37 am
by s.dot
My only complaint about this code is lack of a standard way of doing things. It seems you're calling functions within functions that are defined on various parts of the page. It's okay if it works, but it's a PAIN to add on to code like that.

At the very least, I think you should have your functions defined in a functions.php (or similar) type page, and require it into your script. Then code a very procedural if/else (or switch) statement. Readability of the code would improve 10fold.

To elaborate on that, you could go in a totally OO manner and create user and validation classes.

Either way would make it 10x easier to read, maintain, and update.

Posted: Sun Oct 28, 2007 9:57 am
by Benjamin
To me, it looks like your trying to organize and compartmentalize your code, which is a good thing. You've taken it as far as you can. Your next step is to start learning Object Oriented Programing techniques and design patterns. Lot's of good tutorials out there and it's not really complex.

Posted: Sun Oct 28, 2007 10:54 am
by John Cartwright
I'm surpised nobody suggested his script's vulnerability to SQL injection. All input variables, at minimum, should be passed through mysql_real_escape_string() prior to be added to query.

Posted: Sun Oct 28, 2007 10:56 am
by Benjamin
Jcart wrote:I'm surpised nobody suggested his script's vulnerability to SQL injection. All input variables, at minimum, should be passed through mysql_real_escape_string() prior to be added to query.
Yeah I was looking at the coding style and didn't even notice.