Class advice

[doddsey_65]

ive created this small example class for users to set them up. Before i start to expand i would like some advice on whether its good or not.

 

class user
{
    static $username;
    static $email;
    
    public function __construct($username)
    {
        self::$username = $username;
    }
    
    public function setupUser($username)
    {
        global $link;
        
        $user_query = $link->query(
            "SELECT * FROM ".TBL_PREFIX."users
            WHERE u_username = '$username'"
        )or die($link->print_error());
        
        $row = $user_query->fetch(PDO::FETCH_ASSOC);
        
        if(!$row)
        {
            die('Could not find info for user '.$username);
        }
        else
        {
            self::$email = $row['u_email'];
        }
    }
}

 

So if i wanted to print a users email i could use something like:

 

$user = new user;

echo $user::$email;

 

[RussellReal]

why would you die() on errors, you should pass it to an error object, and check if any errors have accumulated every step of the user verification process.

 

and your use of the :: operator is completely wrong, you don't want the information in a user class to be static, you want it to be dynamic, which you'd use -> operator for.

[KevinM1]

In addition, using 'global' completely kills encapsulation.  The 'global' keyword should be avoided in general, but using it in OOP is contradictory to one of the basic ideas behind OOP.

 

If a fuction/method requires a parameter in order to work then pass it through its argument list.  That's why it's there.

 

Also note that objects can contain other objects.  Simply make your PDO object a member of your User class, like $username.

[ignace]

You can't use self in a non-static context. Like mentioned: using die() is a bad habit even in procedural programming. In OO we use Exception, these are more flexible than a die() statement, once an Exception is thrown it will fall back till the Exception is handled (try..catch..) or PHP will show an uncaught exception error.

 

global is bad, don't use it. It goes against the basic idea of encapsulation.

[doddsey_65]

Thanks for the reply. You mention that using global must be avoided. What about including the $linkvariable using global? So if i want to use a query within a class method i use global $link. Is that still okay?

 

And does making the class variable static mean that it cannot be modified once assigned? That is why i set the username to static, because it wont be changed. Or am i thinking about that wrong?

 

I will change the die statements to exceptions.

 

Thanks for the help so far.

 

[KevinM1]

Thanks for the reply. You mention that using global must be avoided. What about including the $linkvariable using global? So if i want to use a query within a class method i use global $link. Is that still okay?

 

No.  Like I said, do not use 'global'.  You never need to use 'global'.  Take a look at something like:

 

class User
{
   private $username;
   private $email;
   private $db = new PDO(/*args*/);

   public function __construct($username)
   {
      $this->username = $username;
   }

   public function setupUser($username)
   {   
       $user_query = $this->db->query("SELECT * FROM " . TBL_PREFIX . "usersWHERE u_username = '$username'") or die($link->print_error());
        
       $row = $user_query->fetch(PDO::FETCH_ASSOC);
        
       if(!$row)
       {
           die('Could not find info for user ' . $username);
       }
       else
       {
          $this->email = $row['u_email'];
       }
   }
}

 

And does making the class variable static mean that it cannot be modified once assigned? That is why i set the username to static, because it wont be changed. Or am i thinking about that wrong?

 

No, you're way off.  Static means, essentially, class-wide scope as opposed to instance/object scope.  So, something like this:

 

class StaticExample
{
   private static $count = 0;

   public function __construct()
   {
      ++self::$count;
   }

   public function getCount()
   {
      return self::$count;
   }
}

$example1 = new StaticExample();
echo $example1->getCount();

$example2 = new StaticExample();
echo $example2->getCount();

[doddsey_65]

Thanks for the help. I now understand global and inclusion of database info into the class. However i still cant grasp static(sorry) so i will have t look into it further.

[KevinM1]

Try making a simple script that has the StaticExample code I wrote and see what happens.

 

Also, just to beat it into the ground, 'global' is bad regardless of whether or not you're writing OO code or 'normal' code.  If a function/method has an external dependency, pass it in through the argument list.  In other words, never do this:

 

$someVar;

function someFunction()
{
   global $someVar;

   // do something with $someVar
}

 

Instead, do this:

 

$someVar;

function someFunction($someVar)
{
   // do something with $someVar
}

[jcbones]

The reason that we say 'Don't use global' is because you have now tied the function to a particular script, for a particular purpose.

 

Take for instance.

$num = .10;

function discount($price) {
global $num;
return $price - ($price * $num);
}

 

We have now global included a fairly commonly used variable into a function.  Why there is not a problem with it now, being that I defined the variable as .10 for a 10 percent discount.  Pretty straight forward.  I did this so that I would only have to change the discount in one place in the script.  Why would there be anything wrong with that?

 

Here is why.

 

So, I am looking through my library one day for a discount function.  I find this one, I'll just drop it into my script and call it.  It only has one argument, $price.

 

for($num = 0; $num > 10; $num++) {
echo discount($prices[$num]);
}

 

While this may be a simple function, and one that you would instantly see the problem with, there are lots of times the problem will not be this evident.

 

Now why would my included function start returning all these crazy numbers?  I've just caused myself some troubleshooting that would have been prevented by listing $num in the arguments. 

[doddsey_65]

i have taken everything said into consideration and made an example of the final user class which contains a method for registering users.

Let me know what you think and if there are any ways of improving this method. As i said it's just an example and i will be adding to it later.

 

<?php

define('TBL_PREFIX','asf_');

function __autoload($class)
{
    include('./includes/'.$class.'.php');
}

class user
{
    /**
     * Will hold the database class
     *
     * @return void
     */
    public $db;
    
    /**
     * Hash Method
     *
     * @return string
     */
    public $hash = 'md5';
    
    /**
     * Handles the registration of a new user
     *
     * @param array $user_info
     */
    public function registerUser($user_info)
    {
        if(is_array($user_info))
        {
            /**
             * connect to the database
             *
             * @param string $hostname
             * @param string $database
             * @param string $username
             * @param string $password
             */
            $this->db = new db_pdo('localhost',
                                   'asf_forum',
                                   '**********',
                                   '**********');
                                   
            // escape the $user_info array
            $user_info = array_map('addslashes',$user_info);
            
            // hash the password
            switch($this->hash)
            {
                case 'md5':
                    $user_info['password'] = md5($user_info['password']);
                    break;
                    
                case 'sha1':
                    $user_info['password'] = sha1($user_info['password']);
                    break;
            }
            
            // setup the insertion query
            $insert_query = 
                "INSERT INTO ".TBL_PREFIX."users
                (u_username, u_password)
                VALUES
                ('".$user_info['username']."',
                '".$user_info['password']."')
                ";
            
            // execute the query
            $this->db->query($insert_query)    
                or die($this->db->printError($insert_query));
        }
    }
}

$user_info = array();
$user_info['username'] = "test'sUsername";
$user_info['password'] = 'google';

$user = new user;

$user->registerUser($user_info);

[jcbones]

Some people will tell you that calling a class inside a class breaks encapsulation as well, although, I don't share that train of thought.  The argument is that by calling that PDO class, you are tying a specific database type to the class, on the other hand, writing a query inside a class does the same thing.

 

The only thing that I see is, you may want to move your database details to the variable definitions, that way you don't have to change anything in the function, to change database's (TBL_PREFIX as well).

[doddsey_65]

thanks for the help. TBL_PREFIX definition was just put there to make the code work.

[KevinM1]

Some people will tell you that calling a class inside a class breaks encapsulation as well, although, I don't share that train of thought.  The argument is that by calling that PDO class, you are tying a specific database type to the class, on the other hand, writing a query inside a class does the same thing.

 

That's where dependency injection comes in.  Instead of hard coding a particular dependency in a class, you use an object called a dependency injection or inversion of control container to automatically pass (inject) the dependency into the target object.  It works best when all of the potential dependencies share an interface.

 

@OP - you should make your fields $db, $hash, and any others you may add either private or protected.

[ignace]

private $db = new PDO(/*args*/);

 

Oh, how I wish that was possible

[ignace]

JCBones made a very good example as why globals are bad:

 

for($num = 0; $num > 10; $num++) {
echo discount($prices[$num]);
}

 

Functions are meant to be black boxes, that is also how we use them: pass in information, get back the expected information. If it doesn't we have to go figure out why. You could easily solve this if this bug is found during development but what if this bug is discovered after 6 months, or a year? What if your company has expanded and you hired a few recruits? Surely they won't know $num is a global variable used in discount() would they?

 

You could solve this problem partially by writing all global variables UPPERCASE to avoid overwriting it in normal application flow. I say partially because as your application grows more and more functions will be using/change the global variables which is prone to creating bugs.

 

You could solve this even further by grouping all global variables and the functions that use them in a single file but then how do you avoid other functions/libraries of overwriting or writing to your global variable? What if we had a data structure that grouped functions and variables together and did not allow other libraries to overwrite or write to my variable, something like say.. a class?

[doddsey_65]

okay say i have an array $config which is never reassigned a different value. It holds all forum relative variables:

 

$config['board_name'] = 'asf';

$config['board_root'] = './';

 

would i be okay using that as a global when i need to define the board root for a hyperlink.

 

Because i cant use relative urls when using mod rewrite

 

[ignace]

Some people will tell you that calling a class inside a class breaks encapsulation as well

 

Are they mad? Delegation is good practice.

[ignace]

would i be okay using that as a global when i need to define the board root for a hyperlink.

 

It doesn't matter how you turn it, the answer remains: NO. Using an array to store information like configuration variables and languages remain a popular technique among programmers. Passing this array around limits your program to only an array to store this information while at some point you'll realise that using an array to store languages is unmaintainable yet you used the array everywhere in your application so you have to go through the horror of finding and removing unused translations. Using a class instead gives you more flexibility as illustrated below:

 

interface Translator {
  public function get($variable);
}

class ArrayTranslator implements Translator {
  public function __construct(array $array) { $this->_array = $array; }
  public function get($variable) { return $this->_array[$variable]; }
}

 

In my application I use it like:

 

public function setTranslator(Translator $tl)

 

Now i have come to the point where I realise that using an array to store my translations was a bad idea so I'm switching to Gettext and use the awesome tool POEdit to sync my translations.

 

class GetTextTranslator implements Translator {/*actual code omitted*/}

 

In my Factory I replace it with my new GetTextTranslator:

 

class ApplicationFactory {
  public function getConfig() {
    if (is_null($this->_config)) {
      $this->_config = new ArrayConfig(include 'path/to/config.php');
    }
    return $this->_config;
  }
  public function getTranslator() {
    if (is_null($this->_translator)) {
      $this->_translator = new GetTextTranslator('path/to/gettext/directory');
    }
    return $this->_translator;
  }
}

 

I switched my entire application from using an array for translation data to gettext in one single line of code. How long would it have taken you to do the same using global $config or global $translate? Maybe it wouldn't have taken you too much time, if the code was properly written, but can you also switch between languages on the fly without leaving too much of a mess?

 

$translate->_('Hello');
$translate->_('Good morning', 'fr'); // this should always be in French