www.webdeveloper.com
Page 1 of 2 12 LastLast
Results 1 to 15 of 16

Thread: good code or bad

  1. #1
    Join Date
    Aug 2012
    Posts
    40

    good code or bad

    was wondering if there is anything wrong with this code it works fine but i wanna know if there is anything experts or pro would do diff y and what this is just the class that does the processing ect. im gonna write another page to filter input ect before it sends the information here


    <?php

    define("host","localhost");
    define("database_name","yourchoice");
    define("username","root");
    define("password","feind");

    try{

    $conn=new pdo("mysql:host=".host.";dbname=".database_name.";charset=utf8",username,password);
    }
    catch(pdoexception $e)
    {
    echo"sorry the connection to database has failed ".$e;
    }
    ?>



    <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
    <html xmlns="http://www.w3.org/1999/xhtml">
    <head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
    <title>Untitled Document</title>
    </head>
    <body>
    <?php
    session_start();
    require ("config.php");
    class users
    {
    public $uid;
    public $u_name;
    protected $u_password;
    protected $u_info;
    protected $u_salt;
    public function login ($u_name,$u_password)
    {
    $now=time();
    global $conn;
    $this->getsalt($u_name);
    $stmnt=$conn->prepare("select*from users where name=:name and pass=ass");
    $stmnt->execute(array(":name"=>$u_name,"ass"=>sha1($u_password.$this->u_salt)));
    $result=$stmnt->fetch(pdo::FETCH_OBJ);
    if(!$result==null)
    {
    $this->uid=$result->id;
    $this->u_name=$result->name;
    $this->u_password=$result->pass;
    $this->u_info=$result;

    if($this->brute_check($result->id)==false)
    {
    $_session['u_name']=$this->u_name;//registers a session if all checks out

    //deletes the previously failed loggin attempts from table
    $delete_prev_failed_attempts=$conn->prepare("delete from login_attempts where u_id=:id");
    $delete_prev_failed_attempts->execute(array(":id"=>$this->uid));


    echo"<p>login sucessfull</p><br/>ect......whatever page xanda chooses them to see";
    }


    else
    {
    echo"user account locked for the next hour";
    }

    }
    else
    {
    $query=$conn->prepare("select*from users where name=:u_name");
    $query->execute(array(":u_name"=>$u_name));
    $name=$query->fetch(PDO::FETCH_OBJ);
    if($name==null)
    {
    die("<p>Username or password is incorrect........ill bring you back to the login page or ask alex to register you</p>");
    }
    $id=$name->id;
    $insert=$conn->prepare("insert into login_attempts(u_id,time) values(:id,:time)");
    $insert->execute(array(":id"=>$id,":time"=>$now));
    if($this->brute_check($id)==true)
    {
    echo"user account locked for the next hour";
    }
    else
    {
    echo"<p>Username or Password is incorrect........ill bring you back to the login page or ask alex to register you</p>";
    }
    }
    }




    //function to check amount of login attempts with a hour time period refrence table login attemts
    public function brute_check($id)
    {
    global $conn;
    $now=time();
    $hour_ago=$now-(1*60*60);
    $stmnt=$conn->prepare("select time from login_attempts where u_id=:uid and time>=ast1_hour");
    $stmnt->execute(array(":uid"=>$id,"ast1_hour"=>$hour_ago));
    $rows=$stmnt->rowcount();
    if($rows>5)
    return true;
    else
    return false;
    }

    /*this is the login function note to try create a new user ect. ect. via certain things being true*/
    public function register($req_name,$req_pass)
    {
    global $conn;
    $time=time();
    $salt=$time;
    $this->u_salt=$salt;
    $pass=$this->hash_pass($req_pass,$salt);
    $check=$conn->prepare("select*from users where name=:req_name");
    $check->execute(array(":req_name"=>$req_name));
    $rows_check=$check->rowcount();
    if(!$rows_check==null)
    {
    echo"username already taken";
    return false;
    }
    else
    {
    try{
    $insert=$conn->prepare("insert into users(name,pass,salt) values(:req_name,:req_pass,:salt)");
    $insert->execute(array(":req_name"=>$req_name,":req_pass"=>$pass,":salt"=>$salt));
    echo"user created you may now login";
    return true;
    }
    catch(pdoexception $e)
    {
    echo"error ".$e;
    }

    }
    }
    //function to hash passwords
    public function hash_pass($req_pass,$salt)
    {
    $hashed_pass=sha1($req_pass.$salt);
    return $hashed_pass;
    }

    /*im so soryy if this code is cumbersome the point of good code is to be easily understood
    This function Gets the salt from the username and also varifies the user exists*/
    public function getsalt($u_name)
    {
    global $conn;
    $get_salt=$conn->prepare("select name,salt from users where name=:name");
    $get_salt->execute(array(":name"=>$u_name));
    $result=$get_salt->fetch(PDO::FETCH_OBJ);
    if($get_salt->rowcount()>0)
    {
    $this->u_salt=$result->salt;
    return true;
    }
    }

    }
    if($_POST['op']=='login')
    {
    $user=new users;
    $user->login($_POST['name'],$_POST['pass']);
    }
    else if ($_POST['op']=='register')
    {
    $user=new users;
    $user->register($_POST['name'],$_POST['pass']);
    }
    else
    echo"Unknown Request";
    ?>
    </body>
    </html>
    Last edited by zanda; 01-20-2013 at 05:47 PM. Reason: forgot the config

  2. #2
    Join Date
    Aug 2012
    Posts
    40
    lol the editer here posted my named place holers as smilies but i think you all should get it as said it just does the processing all filtering ect will be done on a layer of php before.and also am useing phpmyadmin to create tables ect.the tables are connected and i notice if i tried to alter a feild thats connected inwards it dosnt allow makes sense but what do i do if a user says need his name changed i would have to delete all the data under that users name in forighn tables comments and advice please

  3. #3
    Join Date
    Aug 2004
    Location
    Ankh-Morpork
    Posts
    19,152
    If you post your code withing this forum's [php]...[/php] bbcode tags, it will be much easier for us to read (including keeping indentation, not displaying smilies, and even including syntax highlighting).
    "Please give us a simple answer, so that we don't have to think, because if we think, we might find answers that don't fit the way we want the world to be."
    ~ Terry Pratchett in Nation

    eBookworm.us

  4. #4
    Join Date
    Jun 2006
    Location
    Under your bed
    Posts
    357
    NogDog: Did you mean to include the "not" before disabling smilies? Disabling them seems like a good thing when posting code :P
    The better I get at programming, the more I appreciate arrays. Handy dandy things they are.

  5. #5
    Join Date
    Aug 2004
    Location
    Ankh-Morpork
    Posts
    19,152
    Quote Originally Posted by evenstar7139 View Post
    NogDog: Did you mean to include the "not" before disabling smilies? Disabling them seems like a good thing when posting code :P
    I believe I typed "not displaying" versus "not disabling", unless your browser is displaying (or disabling?) something else.

    However, I did mean to type "within" instead of "withing", but sometimes my fingers have a mind of their own.
    "Please give us a simple answer, so that we don't have to think, because if we think, we might find answers that don't fit the way we want the world to be."
    ~ Terry Pratchett in Nation

    eBookworm.us

  6. #6
    Join Date
    Aug 2012
    Posts
    40
    PHP Code:
    <?php

     define
    ("host","localhost");
     
    define("database_name","yourchoice");
     
    define("username","root");
     
    define("password","feind");

     try{

     
    $conn=new pdo("mysql:host=".host.";dbname=".database_name.";charset=utf8",username,password);
     }
     catch(
    pdoexception $e)
     {
     echo
    "sorry the connection to database has failed ".$e;
     }
     
    ?>



     <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
     <html xmlns="http://www.w3.org/1999/xhtml">
     <head>
     <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
     <title>Untitled Document</title>
     </head>
     <body>
     <?php
     session_start
    ();
     require (
    "config.php");
     class 
    users
     
    {
     public 
    $uid;
     public 
    $u_name;
     protected 
    $u_password;
     protected 
    $u_info;
     protected 
    $u_salt;
     public function 
    login ($u_name,$u_password)
     {
     
    $now=time();
     global 
    $conn;
     
    $this->getsalt($u_name);
     
    $stmnt=$conn->prepare("select*from users where name=:name and pass=ass");
     
    $stmnt->execute(array(":name"=>$u_name,"ass"=>sha1($u_password.$this->u_salt)));
     
    $result=$stmnt->fetch(pdo::FETCH_OBJ);
     if(!
    $result==null)
     {
     
    $this->uid=$result->id;
     
    $this->u_name=$result->name;
     
    $this->u_password=$result->pass;
     
    $this->u_info=$result;

     if(
    $this->brute_check($result->id)==false)
     {
     
    $_session['u_name']=$this->u_name;//registers a session if all checks out

     //deletes the previously failed loggin attempts from table
     
    $delete_prev_failed_attempts=$conn->prepare("delete from login_attempts where u_id=:id");
     
    $delete_prev_failed_attempts->execute(array(":id"=>$this->uid));


     echo
    "<p>login sucessfull</p><br/>ect......whatever page xanda chooses them to see";
     }


     else
     {
     echo
    "user account locked for the next hour";
     }

     }
     else
     { 
     
    $query=$conn->prepare("select*from users where name=:u_name");
     
    $query->execute(array(":u_name"=>$u_name));
     
    $name=$query->fetch(PDO::FETCH_OBJ);
     if(
    $name==null)
     {
     die(
    "<p>Username or password is incorrect........ill bring you back to the login page or ask alex to register you</p>");
     }
     
    $id=$name->id;
     
    $insert=$conn->prepare("insert into login_attempts(u_id,time) values(:id,:time)");
     
    $insert->execute(array(":id"=>$id,":time"=>$now));
     if(
    $this->brute_check($id)==true)
     {
     echo
    "user account locked for the next hour";
     }
     else
     {
     echo
    "<p>Username or Password is incorrect........ill bring you back to the login page or ask alex to register you</p>";
     }
     }
     }




     
    //function to check amount of login attempts with a hour time period refrence table login attemts 
     
    public function brute_check($id)
     { 
     global 
    $conn;
     
    $now=time();
     
    $hour_ago=$now-(1*60*60);
     
    $stmnt=$conn->prepare("select time from login_attempts where u_id=:uid and time>=ast1_hour");
     
    $stmnt->execute(array(":uid"=>$id,"ast1_hour"=>$hour_ago));
     
    $rows=$stmnt->rowcount();
     if(
    $rows>5)
     return 
    true;
     else 
     return 
    false;
     }

     
    /*this is the login function note to try create a new user ect. ect. via certain things being true*/
     
    public function register($req_name,$req_pass)
     {
     global 
    $conn;
     
    $time=time();
     
    $salt=$time;
     
    $this->u_salt=$salt;
     
    $pass=$this->hash_pass($req_pass,$salt);
     
    $check=$conn->prepare("select*from users where name=:req_name");
     
    $check->execute(array(":req_name"=>$req_name));
     
    $rows_check=$check->rowcount();
     if(!
    $rows_check==null)
     {
     echo
    "username already taken";
     return 
    false;
     }
     else
     {
     try{
     
    $insert=$conn->prepare("insert into users(name,pass,salt) values(:req_name,:req_pass,:salt)");
     
    $insert->execute(array(":req_name"=>$req_name,":req_pass"=>$pass,":salt"=>$salt));
     echo
    "user created you may now login";
     return 
    true;
     }
     catch(
    pdoexception $e)
     {
     echo
    "error ".$e;
     }

     }
     }
     
    //function to hash passwords
     
    public function hash_pass($req_pass,$salt)
     {
     
    $hashed_pass=sha1($req_pass.$salt);
     return 
    $hashed_pass;
     }

     
    /*im so soryy if this code is cumbersome the point of good code is to be easily understood
     This function Gets the salt from the username and also varifies the user exists*/
     
    public function getsalt($u_name)
     {
     global 
    $conn;
     
    $get_salt=$conn->prepare("select name,salt from users where name=:name");
     
    $get_salt->execute(array(":name"=>$u_name));
     
    $result=$get_salt->fetch(PDO::FETCH_OBJ);
     if(
    $get_salt->rowcount()>0)
     {
     
    $this->u_salt=$result->salt;
     return 
    true;
     }
     }

     }
     if(
    $_POST['op']=='login')
     {
     
    $user=new users;
     
    $user->login($_POST['name'],$_POST['pass']);
     }
     else if (
    $_POST['op']=='register')
     {
     
    $user=new users;
     
    $user->register($_POST['name'],$_POST['pass']);
     }
     else
     echo
    "Unknown Request";
     
    ?>
     </body>
     </html>
    sorry there is the edited one with tags

  7. #7
    Join Date
    Aug 2012
    Posts
    40
    i edited the code to now have a fail function will post soon but any comments

  8. #8
    Join Date
    Oct 2012
    Posts
    17
    Your code is working very perfectly, but try to optimize it in a simple manner.

  9. #9
    Join Date
    Apr 2010
    Posts
    88
    Actually, this code is not good for many reasons (some of them are the following):

    - No html-php separation
    - You rely on globals for db connection
    - Echo usage inside the class methods
    - You use $_SESSION directly in your class
    - You use die inside you class
    - You should stop the script's execution, using exit or die, on database connection failure
    - You should prefer private scope for variables and methods instead of public or protected. Use protected when your class will be inherited by another class and public when you need to access a method from code outside of the class

  10. #10
    Join Date
    Aug 2012
    Posts
    40
    Quote Originally Posted by gvre View Post
    Actually, this code is not good for many reasons (some of them are the following):

    - No html-php separation
    - You rely on globals for db connection
    - Echo usage inside the class methods
    - You use $_SESSION directly in your class
    - You use die inside you class
    - You should stop the script's execution, using exit or die, on database connection failure
    - You should prefer private scope for variables and methods instead of public or protected. Use protected when your class will be inherited by another class and public when you need to access a method from code outside of the class
    firstly thank you for your reply and sorry for assuming you would know i actually sace the db connection and its test in config.php and that is why i refer to the connection as a global and i found this the best method for me.

    also the echoes you see are stricly for my purposes they will be replaced with redirections.
    also i didnt see the need to filter the session in the class seeing as now input from the user reaches that part of the code it is used for was i wrong?

    and thanks for the tips but i have sepeerated my html do you see anything that is to be displad on the web page their?

    for the protected stuff im new to this so just trying to play it a little safe will do.
    any other things you said their were a few more?

    and how do you suggest i optimpize?
    i can go through and clean it up

  11. #11
    Join Date
    Apr 2010
    Posts
    88
    As I can see, your html and php code are in the same file. You should move all classes to their own files (e.g. user.php for class User).

    Globals are the easy way, but not the right way. Each class should be independent from globals. A better way to use the db connection inside your class is the use of dependency injection. For example:

    PHP Code:
    // constructor injection
    public function __construct(PDO $db)
    {
            
    $this->db $db;

    or

    PHP Code:
    // method injection
    public function setDatabase(PDO $db)
    {
            
    $this->db $db;

    You shouldn't use echo, exit, die or redirects inside your class. It is better to return a value from methods and use it inside your controllers to do redirect or whatever you want.

    The same applies to the $_SESSION use. What will happen if oneday you decide not to use sessions?

    Generally, every class should be as much independent as it can be.

    ps. $_POST['op'] could throw a notice if op does not exist in $_POST array. A better way to get variables from arrays is the use of empty or isset. e.g.

    PHP Code:
    if (isset($_POST['op']) && $_POST['op'] == 'login'

  12. #12
    Join Date
    Aug 2012
    Posts
    40
    Quote Originally Posted by gvre View Post
    As I can see, your html and php code are in the same file. You should move all classes to their own files (e.g. user.php for class User).

    Globals are the easy way, but not the right way. Each class should be independent from globals. A better way to use the db connection inside your class is the use of dependency injection. For example:

    PHP Code:
    // constructor injection
    public function __construct(PDO $db)
    {
            
    $this->db $db;

    or

    PHP Code:
    // method injection
    public function setDatabase(PDO $db)
    {
            
    $this->db $db;

    You shouldn't use echo, exit, die or redirects inside your class. It is better to return a value from methods and use it inside your controllers to do redirect or whatever you want.

    The same applies to the $_SESSION use. What will happen if oneday you decide not to use sessions?

    Generally, every class should be as much independent as it can be.

    ps. $_POST['op'] could throw a notice if op does not exist in $_POST array. A better way to get variables from arrays is the use of empty or isset. e.g.

    PHP Code:
    if (isset($_POST['op']) && $_POST['op'] == 'login'
    thanks for the info.
    i agree with you for all except that for the the dependensy in this case as i relly saw the benifits of using the connection as a global in a seperate config file when i had to move from my test local host to a remote test server.

    dreamweaver sets up the page like that when you ask it for a php file so i assumed it was ok to leave those html tags ill remove them.

    thanks for all your help could you give an example of how you would use the session as i also dont see the bad of if i had to change from the sessions are you saying where i initialised my session i should have captured it in say a variable?
    Last edited by zanda; 02-22-2013 at 03:08 PM.

  13. #13
    Join Date
    Feb 2013
    Posts
    46
    Dreamweaver isn't helping you like you might think... Bare minimum for PHP frameworks is to follow PSR-0:
    https://github.com/php-fig/fig-stand...epted/PSR-0.md

  14. #14
    Join Date
    Feb 2013
    Posts
    46
    You know you are doing something wrong in terms of templating if you ever use the '?>' closing tag, which is not required unless mixing HTML and PHP.

  15. #15
    Join Date
    Jul 2003
    Location
    The City of Roses
    Posts
    2,503
    The first half of the article From Flat PHP to Symfony2 could help you. It takes you step by step from the kind of code you have now to a more structured and organized architecture.
    for(split(//,'))*))91:+9.*4:1A1+9,1))2*:..)))2*:31.-1)4131)1))2*:3)"'))
    {for(ord){$i+=$_&7;grep(vec($s,$i++,1)=1,1..($_>>3)-4);}}print"$s\n";

Thread Information

Users Browsing this Thread

There are currently 1 users browsing this thread. (0 members and 1 guests)

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •  
HTML5 Development Center



Recent Articles