First attempt of using switch

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
shivam0101
Forum Contributor
Posts: 197
Joined: Sat Jun 09, 2007 12:09 am

First attempt of using switch

Post 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();
?>
User avatar
Kieran Huggins
DevNet Master
Posts: 3635
Joined: Wed Dec 06, 2006 4:14 pm
Location: Toronto, Canada
Contact:

Post by Kieran Huggins »

isset() returns TRUE or FALSE, not the value of the variable it checks
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post by Benjamin »

That's fine, he's using a ternary operator.
User avatar
Kieran Huggins
DevNet Master
Posts: 3635
Joined: Wed Dec 06, 2006 4:14 pm
Location: Toronto, Canada
Contact:

Post by Kieran Huggins »

oops :oops: totally missed that paren.

/ignore
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

shivam0101, you haven't explained anything. Please do so.
shivam0101
Forum Contributor
Posts: 197
Joined: Sat Jun 09, 2007 12:09 am

Post 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
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post 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.
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post 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.
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Post 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.
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Post 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.
Post Reply