Advanced oop concept

Question

I am writing database api for my company project.

we have 4 classes in database api.

1. user class (include information about user )
2. gib class (include gib information)
3. card class(
   1. each user owns one card
   2. each user owns two gib card
   3. one user can give card to other user any card user or gib card .
4. connection
   1. there can be many connection of user card or gib card in particular gib.
5. db class for making db query.

This page where i am calling the methods of classes

<?php
 include_once "config.php";
 foreach($_REQUEST as $key=>$value){
     $$key=$value;
 }

if($pagelength=='')$pagelength=10;// default page length
//because by username can update the other users data 
if($username!='' && $command!='saveuser'){
    $user=User::getuserarray($username);
    $userid=$user['userid'];
}
if($isowner!=''){
    $user=User::getuserarray($isowner);
    $userid=$user['userid'];
}
$users = array (
    "userid"  => $userid,
    "username" => $username,
    "firstname"   => $firstname,
    "lastname"   => $lastname,
    "password"   => $password,
    "email"   => $email,
    "photo"   => '',
    "url"   => $url,
    "avatar_url"   => $avatar_url,
    "thumb"   => $thumb,
    "crop_url"   => $crop_url,
    "crop_position"   => $crop_position
);
$getcard=array(
    'cardid'=>$cardid,
    'card_type'=>$card_type,
    'status'=>$status,
    'userid'=>$userid,
    'userid_to'=>$userid_to,
    'message'=>$message,
    'name'=>$name,
    'pagelength'=>$pagelength,
    'pagenumber'=>$pagenumber,
    'fields'=>$fields
);
$getuser=array(
    'userid'=>$userid,
    'name'=>$name,
    'pagelength'=>$pagelength,
    'pagenumber'=>$pagenumber,
    'fields'=>$fields

);
$getgib=array(
    'gibid'=>$gibid,
    'userid'=>$userid,
    'isowner'=>$isowner,
    'description'=>$description,
    'tagline'=>$tagline,
    'name'=>$name,
    'pagelength'=>$pagelength,
    'pagenumber'=>$pagenumber,
    'fields'=>$fields
);
$getlink=array(
    'gibid'=>$gibid,
    'view'=>$view,
    'userid'=>$userid,
    'cardid'=>$cardid,
    'name'=>$name,
    'pagelength'=>$pagelength,
    'pagenumber'=>$pagenumber,
    'fields'=>$fields
);
switch($command){
    case 'login':
        $user=new User(array());
        $msg=$user->login($username,$password);
        break;
    case 'logout':
        $user=new User(array('userid'=>$userid));
        $user->logout($id_session);
        break;
    //~ case 'isowner':
        //~ $gib=new Gib(array('gibid'=>$gibid));
        //~ $user=new User(array('userid'=>$userid));
        //~ if($user->validateUserid()!=true) return $msg;
        //~ if($gib->validateGibid()!=true) return $msg;
        //~ $msg=$gib->isowner($userid,$gibid);
        //~ break;
    case 'saveuser':
        $user=new User($users);
        if($userid==''){
            //checking if blank fields
            $msg=$user->validate();
            if($msg!='ok')break;
            $msg=$user->adduser();
        }else{
            $msg=$user->updateuser();
        }
        break;
    //~ case 'updateuser':
        //~ $user=new User($users);
        //~ $user->updateuser();
        //~ break;
    case 'changepassword':
        $user=new User($users);
        $msg=$user->changepassword($old_pswd,$new_pswd,$confirm_pswd);
        break;
    case 'changeimage':
        $user=new User($users);
        $user->changeimage($thumb,$url,$avatar_url,$crop_position,$crop_url);
        break;

    case 'getuser':
        $user=new User(array('userid'=>$userid));
        $msg=$user->getuser($getuser);
        break;
    /*----------link command -----------*/
    case 'getlink':
        $connection=new Connection(array());

        $connections=$connection->getlink($getlink);
        $connections=array('links'=>$connections);
        echo $msg=json_encode($connections);
        break;  
    case 'createlink':
        $user=new User($users);
        $msg=$user->validateUserid();
        if($msg!='ok') break;
        $connection=new Connection(array());
        $msg=$connection->createlink($getlink);
        break;
    case 'updatelink':
        $connection=new Connection(array('linkid'=>$linkid));
        $msg=$connection->validate();
        if($msg!='ok')break;
        $connection->updatelinkposition($positionx,$positiony);
        break;  
    case 'deletelink':
        $connection=new Connection(array('linkid'=>$linkid));
        $msg=$connection->validate();
        if($msg!='ok')break;
        $connection->deletelink();
        break;
    //~ case 'cardsend_validate':
        //~ $card=new Card(array('cardid'=>$cardid));
        //~ $msg=$card->cardSendValidate($userid_to);
        //~ break;
    case 'getcard':
        $card=new Card(array('cardid'=>$cardid));
         $cards=$card->getcardlist($getcard);
        if($cardid=='')
            echo $msg=$cards;
        else
            echo $msg=json_encode($cards);
        break;
    case 'givecard':
        $card=new Card(array('cardid'=>$cardid));
        $msg=$card->validate();
        if($msg!='ok')break;
        $user=new User(array('userid'=>$userid_to));
        $msg=$user->validateUserid();
        if($msg!='ok') break;
        $msg=$card->givecard($getcard);
        break;
    case 'cardar'://accept reject
        $card=new Card(array('cardid'=>$cardid));
        $msg=$card->validate();
        if($msg!='ok')break;
        $msg=$card->acceptReject($getcard);
        break;
    case 'deletecard':
        $card=new Card(array('cardid'=>$cardid));
        $msg=$card->validate();
        if($msg!='ok')break;
        $msg=$card->delete_card();
        break;
    case 'getgib':
        $gib=new Gib(array('gibid'=>$gibid));
        $gibs=array('success'=>true,'gibs'=>$gib->getgibs($getgib));
        echo $msg=json_encode($gibs);
        break;
    case 'savegib':
        $gib=new Gib(array('gibid'=>$gibid));
        $user=new User($users);
        $msg=$user->validateUserid();
        if($msg!='ok') break;
        if($gibid==''){
            $gibA=$gib->creategib($name,$type,$userid,$description,$tagline,$gib_background);
            $gibid=$gibA['gibid'];
            $gib->gibid=$gibid;
            $msg= 'gib created Successfully';
        }else{
            $gib->updategib($name,$description,$tagline,$gib_background);
            $msg= 'update Form submission complete';
        }
        $arrayjson1=array(
            'success' => true,
            'message'=>$msg
        );
        $gibA=$gib->getDetail();
        echo $msg= formjson(array(),$gibA,$arrayjson1); 
        break;


    default:
        break;
}

?>

<script language="javascript">
window.location.href="enterspace.php?msg=<?php echo urlencode($msg); ?>";
</script>

Here i am showing only you the user class for suggestion

<?php
Class User{

    var $userid;
    var $username;
    var $firstname;
    var $password;
    var $email;
    var $photo;
    var $avatar;
    var $thumbnail;
    var $crop_url;
    var $crop_position;



    function User($users){
        $this->userid=$users['userid'];
        $this->username= $users['username'];
        $this->firstname=$users['firstname'];
        $this->lastname=$users['lastname'];
        $this->password= $users['password'];
        $this->email=$users['email'];
        $this->photo= $users['photo'];
        $this->avatar= $users['avatar'];
        $this->thumbnail= $users['thumbnail'];
        $this->crop_url= $users['crop_url'];
        $this->crop_position= $users['crop_position'];

    }
    function validateUserid(){
        if($this->userid==''){
            $msg=geterrormsg(1);
            $arrayjson1=array(
                        'success' => false,
                        'message'=>$msg,
                        'error_code'=>'1');
            echo $msg= formjson(array(),array(),$arrayjson1);
            return $msg;

        }
        $sql="select count(*) from users where userid=?";
        $count=Db::getValue($sql,$this->userid);
        //User not exist
        if($count<=0){
            $msg=geterrormsg(1);
            $arrayjson1=array(
                        'success' => false,
                        'message'=>$msg,
                        'error_code'=>'1');
            echo $msg= formjson(array(),array(),$arrayjson1);
            return $msg;
        }
        return 'ok';

    }   

    function validate(){
        //checking if blank fields
        if($this->firstname=='' or $this->lastname=='' or $this->password=='' or $this->email=='' or $this->username==''){
            $msg=geterrormsg(6);
            $arrayjson1=array(
                        'success' => false,
                        'message'=>$msg,
                        'error_code'=>'6'
                    );
            echo $msg= formjson(array(),array(),$arrayjson1);   
            return false;
        }
        if($this->userid==''){
            $totalUsers = Db::getValue('SELECT COUNT(username) FROM users where username = ?',$this->username);
            if($totalUsers>0){
                //username already exist
                $msg=geterrormsg(5);
                $arrayjson1=array(
                        'success' => false,
                        'message'=>$msg,
                        'error_code'=>'6'
                    );
                echo $msg= formjson(array(),array(),$arrayjson1);
                return $msg;
            }
            $totalUsers = Db::getValue('SELECT COUNT(email) FROM users where email = ?',$this->email);
            if($totalUsers>0){
                $msg=geterrormsg(4);
                    //email already exist
                $arrayjson1=array(
                        'success' => false,
                        'message'=>$msg,
                        'error_code'=>'4'
                    );
                echo $msg= formjson(array(),array(),$arrayjson1);   
                return $msg;
            }
        }
        return 'ok';
    }
    function login($loginUsername,$loginpassword){
        //query for checking user username  exist or not 
        $sql="select count(*) from users where username=?";
        $count=Db::getValue($sql,$loginUsername);
        if($count<=0){
            $msg=geterrormsg(1);
            $arrayjson1=array(
                        'success' => false,
                        'message'=>$msg,
                        'error_code'=>'1'
                    );
            echo $msg= formjson(array(),array(),$arrayjson1);   
            return $msg;
        }
        $sql="select AES_DECRYPT(password,'text') as password,userid from users where username=?";
        $row=Db::getRow($sql,$loginUsername);
        if(is_array($row))  extract($row);
        $this->userid=$userid;
        //for checking password
        if($loginpassword != $password){
            $msg=geterrormsg(2);
            $arrayjson1=array(
                        'success' => false,
                        'message'=>$msg,
                        'error_code'=>'2'
                    );
            echo $msg= formjson(array(),array(),$arrayjson1);   
            return $msg;
        }

        //query for checking user exist  in session table with status =1

        //~ $ses_id = session_id(); 
        //~ $_SESSION['username']=$loginUsername;
        //~ $_SESSION['userid']=$userid;
        //~ $sql="DELETE FROM sessions WHERE userid=? or id_session=?";
        //~ Db::execute($sql,array($userid,$ses_id));
            //~ $sql="INSERT INTO sessions  (id_session,    userid,     START, 
        //~ END,    STATUS,     last_update )   VALUES  (?, ?,current_timestamp,    'end',  '1',    current_timestamp   );";
        //~ Db::execute($sql,array($ses_id,$userid));       

        $gibid=$this->getDefaultGibId();        
        //making user array
        $row=$this->getDetail();
        unset($row['updatedon'],$row['createdon']);

        $gib=new Gib(array('gibid'=>$gibid));
        //~ $gibs=$gib->getgibs($userid,'',5);
        $systemgibid=$gib->systemgibid();
        $arrayjson1=array(
                    'success' => true,
                    'message'=>'User logged in successfully',
                    'gibid'=>$gibid,
                    'systemgibid'=>$systemgibid
                );
        echo $msg= formjson(array(),$row,$arrayjson1);  
        return $msg;    

    }
    function getDefaultGibId(){
        $sql="SELECT referid FROM cards WHERE userid_from=? AND userid_to=?  and card_type='A' " ;
        $gibid=Db::getValue($sql,array($this->userid,$this->userid));
        return $gibid;
    }

    //making user array
    function getDetail(){
        $sql="select  * from users where userid=?";
        $row=Db::getRow($sql,$this->userid);
        unset($row['password']);
        return $row;
    }
    // for creating new user 
    // will have one user entry , one profile card entry in card table , one system gib 
    function adduser(){
         $sql = "INSERT INTO users  (username, firstname,   lastname, PASSWORD,email,createdon  )
            VALUES  (?, ?, ?, AES_ENCRYPT(?,'text'),?,current_timestamp);";
        Db::execute($sql,array($this->username,     $this->firstname, $this->lastname,$this->password,$this->email));
        $this->userid=Db::getLastInsertId();
        //make profile card in cards table...
        $sql="INSERT INTO cards( userid_from, userid_to,card_type, referid,status,createdon)VALUES(?, ?, ?, ?, ?,current_timestamp)";   
        Db::execute($sql,array($this->userid,$this->userid,'V',$this->userid,'A'));

        $id_card=Db::getLastInsertId();
        $gib=new Gib(array());
        $systemgibid=$gib->systemgibid();
        //make system gib card in cards table...
        $sql="INSERT INTO cards(userid_from,    userid_to,  referid, 
            card_type   ,status,createdon)VALUES(?,  ?, ?,?,?,current_timestamp )"  ;   
        Db::execute($sql,array($this->userid,$this->userid,$systemgibid,'A','A'));  
        $this->firstname=$this->firstname."'s Gib";
        //create gibs define in connection.php type D for default gib
        $gib->creategib($this->firstname,'D',$this->userid,'','');
        $arrayjson=array();
        $row=$this->getDetail();

        $arrayjson1=array(
            'success' => true,
            'message'=>'Registered in successfully',
            'username'=>$this->username
        );
        echo $msg= formjson($arrayjson,$row,$arrayjson1);   
        return $msg;
    }
    function updateuser(){
        $sql="UPDATE users SET firstname = ?,   lastname = ?, email=?   WHERE userid = ? ";
        Db::execute($sql,array($this->firstname,$this->lastname,$this->email,$this->userid));
        $user=$this->getDetail();
        $arrayjson1=array(
            'success' => true,
            'message'=>'form submission complete'
        );
        echo $msg= formjson(array(),$user,$arrayjson1); 
        return $msg;
    }
    function changepassword($old_pswd,$new_pswd,$confirm_pswd){

        $sql="select count(*) from users where password=AES_ENCRYPT(?,'text') and userid=? ";
        $count=Db::getValue($sql,array($old_pswd,$this->userid));
        if($count<=0){
            $msg=geterrormsg(28);
            //old passwod not matched
            $arrayjson1=array(
                'success' => false,
                'message'=>$msg,
                'error_code'=>'28'
            );
            echo $msg= formjson(array(),array(),$arrayjson1);   
            return $msg;
        }
        //checking  confirm and new password 
        if($new_pswd!=$confirm_pswd){
            $msg=geterrormsg(29);
            $arrayjson1=array(
                'success' => false,
                'message'=>$msg,
                'error_code'=>'29'
            );
            echo $msg= formjson(array(),array(),$arrayjson1);   
            return $msg;
        }
        $sql="UPDATE users  SET PASSWORD = AES_ENCRYPT(?,'text')
            WHERE userid = ? ; ";
        Db::Execute($sql,array($new_pswd,$this->userid));
        $arrayjson1=array(
            'success' => true,
            'message'=>'password changed successfully .',
            'userid'=>$this->userid
        );
        echo $msg= formjson(array(),array(),$arrayjson1);   
        return $msg;
    }
    function changeimage(){

        $sql="UPDATE users SET  thumbnail=? ,url=?, avatar=?,crop_position=?,   crop_url=?  WHERE userid = ? ";
        Db::Execute($sql,array($this->thumbnail,$this->url,$this->avatar,$this->crop_position,$this->crop_url,$this->userid));
        $user=$this->getDetail();
        $arrayjson1=array(
            'success' => true,
            'message'=>'form submission complete'
        );
        echo $msg= formjson(array(),$user,$arrayjson1);     
    }
    function logout($id_session){
        //~ $sql="delete from sessions where userid=? and id_session=?";
        //~ Db::Execute($sql,array($this->userid,$id_session));

        //~ $sql="delete from occupants where userid=?";
        //~ Db::Execute($sql,$this->userid);

        //~ unset($_SESSION['username']);
        //~ session_destroy(); // start up your PHP session! 


        //~ echo $msg= "{'success': 'true','message':'logout successfully','userid':'$userid'}";
        return $msg;
    }
    function getuser($getuser){
        $userid=$getuser['userid'];
        $name=$getuser['name'];
        $pagelength=$getuser['pagelength'];
        $pagenumber=$getuser['pagenumber'];
        $fields=$getuser['fields'];


        $condition = '';
        $query=array();

        if($userid !='') {
            $condition .= " and userid=? ";
            array_push($query,$userid);
        }
        if($name!=''){
            $condition .= " and concat_ws('  ',firstname,lastname)  like ?";
            array_push($query,"%".$name."%");
        }

        if($pagelength!=''){
            $limitpagelength="limit $pagelength";
        }

        if($pagenumber!=''){
            $pagenumber=$pagelength*($pagenumber-1);
            $pagenumber="offset $pagenumber";
        }
        if($fields=='*'){
            $fields=",users.*";
        }elseif($fields!=''){
            $fields=",".$fields;
        }


        $sql="select  userid,concat_ws(' ',firstname,lastname) as name $fields 
            from users where 1=1   $condition order by updatedon $limitpagelength $pagenumber";
        $row=Db::getResult($sql,$query);

        $user=array();
        for($i=0;$i<count($row);$i++){
            unset($row[$i]['password']);
            extract($row[$i]);
            $this->userid=$userid;
            $row[$i]['gibid']=$this->getDefaultGibId();
            array_push($user,$row[$i]);
        }


        $arrayjson1=array(
            'success' => true,
            'message'=>'User data successfully',
            'users'=>$user
        );
        echo $msg= formjson(array(),array(),$arrayjson1);   
        return $msg;
    }
    //making user array
    function getuserarray($username){
        $sql="select  * from users where username=?";
        $row=Db::getRow($sql,$username);
        unset($row['password']);
        return $row;
    }


}


?>

Please suggest me how can i improve my code by implementing advanced and all oops concepts. If you find this question vague , please suggest the improvement. Thanks

Answer 1

This

 foreach($_REQUEST as $key=>$value){
     $$key=$value;
 }

can be shortened to

extract($_REQUEST);

However, doing so is insecure for two reasons:

1. Using $_REQUEST is insecure unless you know the order in which data was merged into it
2. Extracting variables might potentially overwrite existing important variables.

Since you are already doing OOP, I suggest to introduce a Request object into your application and access any Request parameters through it's API. Instead of extracting Request data into array subsets, just pass the entire Request object to any functions or methods that need it, e.g. instead of

$connections = $connection->getlink($getlink);

do

$connections = $connection->getlink($request);

In addition to that, @mario has a good point with replacing that switch/case block. Basically, the code you show is a FrontController that determines what to do next from the input received. So you could make all these command strings into classes of their own, e.g.

class LoginCommand
class SaveUserCommand
…

and then just instantiate the command and run an interface method, e.g. a method all of these commands must implement, on the instance. Then you could replace your switch/case with

$request         = new Request($_GET, $_POST);
$commandClass    = $request->getCommand() . 'Command';
$commandInstance = new $commandClass($request);
$commandInstance->execute();

Since you have several commands working on the same objects, e.g. "getLink" and "createLink" all work on Connection, you might also want to consider to group these related commands into a PageController.

Whether you introduce a second argument "controller" to do something like this

$controllerClass = $request->getController();
$controller      = new $controllerClass($request);

call_user_func(array($controller, $request->getCommand()));

or use a Router to map commands to appropriate methods in the controller

$router          = new Router('routes.ini');
$controllerClass = $router->mapCommandToControllerClass($request);
$controller      = new $controllerClass($request);

call_user_func(array($controller, $request->getCommand()));

is up to you.

If you do it this way, you already quite close to MVC (and ignore quibble about whether it's MVP or HMVC because it's on the web). The most important factor of all these is separating business logic from presentation layer anyway.

I won't comment on your classes except for that I do not believe in ActiveRecord and think you should separate the code that queries the database from the code that does business operations on the data capsuled in these classes.

On a final note, I find your code with the lack of spaces rather hard to read. Consider following a Code Convention like PEAR. And use PHP_CodeSniffer to make sure you are following that convention.

Disclaimer: none of the above is considered production code. These are just examples to give you an idea on how to improve your application architecture.

Answer 2

Two things. Derive the User class (and probably the others too) from ArrayObject, possibly with ArrayObject::ARRAY_AS_PROPS. This way you can use the data as-is, can save manual property filling.

class User extends ArrayObject {

    function User($users) {   // __construct
        parent::__construct($users, 2);

This way you can access $this["username"] and $this->username alike. Might benefit handling elsewhere.

For the first script, try to replace the lengthy switch with a class and methods. You can use call_user_func(array("modules", $command)) to invoke them.

Thumbs up on using prepared statements, btw!