Can I simplify my class coding?

[chilli11]

I am just wondering if my usual way of creating classes can be simplified as I always seem to be repeating a lot of code and db field names in both the class and the page where it is used??

An snippet from one of my current classes:

class Dogs extends DatabaseConnect {

    public $dog_id;
    public $account_id;
      public $main_handler_id;
    public $owner_id;
    public $kc_name;
    public $pet_name;
    public $kc_number;
    public $grade_id;
    public $height_id;
    public $breed_id;
    public $sex;
    public $dob_day;
    public $dob_month;
    public $dob_year;
    public $aw_type;
    public $grade_updated;
    
    //champ wins publiciable
    public $champ_win_id;
    public $winshow;
    public $windate;
    public $winclass;
    
    //grade change publiciables
    public $grade_changes_id;
    public $previous_grade_id;
    public $new_grade_id;
    public $win_date;
    
    private $order_by='';
    
    public function Dogs ()
    {
        $this->order_by='kc_name, pet_name asc';
        
        $connection=$this->DatabaseConnect();
    }
    
    // Check if dog is already registered on account
    public function checkExists()
    {
        if (!empty($this->pet_name) || !empty($tnhis->kc_name)) {
            $strSQL = mysql_query("select dog_id from scl_dogs where (kc_name='".$this->kc_name."' OR pet_name='".$this->pet_name."') and account_id='".$_SESSION['account_id']."'");
            
            list($this->dog_id) = mysql_fetch_row($strSQL);
        }
    }
    
    // Get all dogs registered to the account
    public function getDogs()
    {
        $strSQL="select * from scl_dogs where account_id='".$_SESSION['account_id']."' ORDER BY '".$this->order_by."'";
        
        $this->executeQuery($strSQL); 
        
    }
    
    // Get all details for dog
    public function getDetails()
    {
        $strSQL = mysql_query("select * from scl_dogs where account_id='".$_SESSION['account_id']."'");
        
        
        list($this->dog_id,
        $this->account_id,
        $this->main_handler_id,
        $this->owner_id,
        $this->kc_name,
        $this->pet_name,
        $this->kc_number,
        $this->grade_id,
        $this->height_id,
        $this->breed_id,
        $this->sex,
        $this->dob_day,
        $this->dob_month,
        $this->dob_year,
        $this->aw_type,
        $this->updated) = mysql_fetch_array($strSQL);
    }
        
    public function addDog ()
    {
        mysql_query("insert into scl_dogs (
                     account_id,
                     main_handler_id,
                     owner_id,
                     kc_name,
                     pet_name,
                     kc_number,
                     grade_id,
                     height_id,
                     breed_id,
                     sex,
                     dob_day,
                     dob_month,
                     dob_year,
                     aw_type)
                     VALUES (
                    '".$_SESSION['account_id']."',
                    '".$this->main_handler_id."',
                    '".$this->owner_id."',
                    '".$this->kc_name."',
                    '".$this->pet_name."',
                    '".$this->kc_number."',
                    '".$this->grade_id."',
                    '".$this->height_id."',
                    '".$this->breed_id."',
                    '".$this->sex."',
                    '".$this->dob_day."',
                    '".$this->dob_month."',
                    '".$this->dob_year."',
                    '".$this->aw_type."')") or die(mysql_error()) ;
        
        $this->dog_id=mysql_insert_id();
        
    }
    
    public function editDog ()
    {
        mysql_query("update scl_dogs set 
                    main_handler_id='".$this->main_handler_id."',
                    owner_id='".$this->owner_id."',
                    kc_name='".$this->kc_name."',
                    pet_name='".$this->pet_name."',
                    kc_number='".$this->kc_number."',
                    grade_id='".$this->grade_id."',
                    height_id='".$this->height_id."',
                    breed_id='".$this->breed_id."',
                    sex='".$this->sex."',
                    dob_day='".$this->dob_day."',
                    dob_month='".$this->dob_month."',
                    dob_year='".$this->dob_year."',
                    aw_type='".$this->aw_type."',
                    updated=NOW()
                    WHERE dog_id='".$_SESSION['dog_id']."'") or die(mysql_error()) ;
        
    }
}

If I am using the class to load a dog’s details for viewing/editing, I call the class and then have to repeat all the variable names again, them same when sending the information to be updated.

I am just thinking that there must be a better way to do this without having to write the same code/field names over and over and also if there is I could re-use in other classes.

[Lemon_Juice]

To me this looks like a not fully successful attempt to create an Active Record class which deals with data from a single db row. As for the repeated field names, you might try putting them all into an array in the constructor and later when you do selects, inserts or updates, use foreach to iterate over all of them - this way you won’t have to repeat them. Also remember that when you do SELECT * then you can get all field names from mysql_fetch_assoc() - you can also use that information (but this may be a bit ugly solution).

What I do is I have a simple Active Record system that has one class for each db table and I have dealt with typing all the code by creating a gererator - a php script that gets the tables with all their field definitions from db and automatically creates classes with all properties defined, getters, etc. This way I can have all my db classes ready withing seconds after creating all my db tables.

I don’t know how you use this class exactly but for admin systems where I know the site owners will not try to hack the system, I allow myself the luxury of defining all fields in html forms in arrays. For example, all input fields have names like this:

dog[account_id]
dog[main_handler_id]
dog[owner_id]
dog[kc_name]
dog[pet_name]
…

Then $_POST[‘dog’] contains all fields for the dog table straight from the form and I can pass them all easily to an insert statement without having to type them again. However, on a public site I don’t do this because someone could hack the html form and manipulate the db by adding fields that should not be there so manually get each form value so that I can have absolute control what goes into the db.

And last thing - don’t forget about mysql_real_escape_string() - you should use this function in this class for every string value you pass to the db for security reasons. You also should sanitize numeric and null values appropriately before injecting them into sql.

[Michael_Morris1]

mysql_real_escape_string is not reliable and can be tricked. Use PDO.

[Lemon_Juice]

@Michael_Morris

mysql_real_escape_string is not reliable and can be tricked. Use PDO.

This sounds provoking. Can you tell us more how and why it is not reliable?

[oddz]

Initially I thought the same thing as Lemon Juice upon initial inspection. However, a significant advantage of the ActiveRecord is automation for common tasks, which you have none of. Therefore, I would recommend reformatting the entire thing into a Mapper. Domain and business logic may as well be completely separate considering there is really little advantage to an ActiveRecord without automation of common tasks acorss all tables, ie. save, find, etc. There is just a lot of repetition in common tasks, that would be eliminated using an ActiveRecord, query builder. Not that he repetition is a bad thing, because your focused on busness specific tasks – which is good. However, that doesn’t really lend itself toward an ActiveRecord, which is in most cases more powerful with generic actions across all entities in a system.

[oddz]

This is an example of ActiveRecord usage. Once the table mapping has been defined all the methods for finding and saving are automated. So with any new table once the class has been defined the system picks up the actual labor of the finding everything, without needing to write repetitive SQL.

</php
class Dog extends \\\\ActiveRecord {

	public static 
	
	// Table mapping
	$table = 'scl_dogs';
	
	// primary key column
	,$id = 'dog_id';
	
	// available table fields
	,$fields = array(
   	 	'dog_id'
    	,'account_id'
    	,'main_handler_id'
    	,'owner_id'
    	,'kc_name'
    	,'pet_name'
    	,'kc_number'
    	,'grade_id'
    	,'height_id'
    	,'breed_id'
    	,'sex'
    	,'dob_day'
    	,'dob_month'
    	,'dob_year'
    	,'aw_type'
    	,'grade_updated'
    
   	 	//champ wins publiciable
   	 	,'champ_win_id'
    	,'winshow'
    	,'windate'
    	,'winclass'
    
   	 	//grade change publiciables
    	,'grade_changes_id'
    	,'previous_grade_id'
    	,'new_grade_id'
    	,'win_date'
    );

}

// Get all dogs registered to the account and sort
$dogs = Dog::find(array(
	'account_id'=>908
	,'sort'=>array(
		'c_name'=>'asc'
		,'pet_name'=>'asc'
	)
));

// Get all details for dog
$dog = Dog::find(array('account_id'=>876))->get(0);

// Save new dog
$dog = new Dog();

$dog->account_id 		= $_SESSION['account_id'];
$dog->main_handler_id 	= $_POST['handler'];
$dog->owner_id 			= $_POST['owner_id'];
$dog->kc_name 			= $_POST['kc_name'];
$dog->pet_name 			= $_POST['pet_name'];
$dog->kc_number			= $_POST['kc_name'];
$dog->grade_id 			= $_POST['grade_id'];
$dog->height_id 		= $_POST['height_id'];
$dog->breed_id 			= $_POST['breed_id'];
$dog->sex 				= $_POST['sex'];
$dog->dob_day 			= $_POST['dob_day'];
$dog->dob_month 		= $_POST['month'];
$dog->dob_year 			= $_POST['year'];
$dog->aw_type 			= $_POST['type'];

try {
	$dog->save();
} catch(\\ActiveRecord\\Exception\\UnableToSave $e) {
	/* set errors */
}
?>

[chilli11]

@Lemon_Juice

And last thing - don’t forget about mysql_real_escape_string() - you should use this function in this class for every string value you pass to the db for security reasons. You also should sanitize numeric and null values appropriately before injecting them into sql.

Thank you for the information - I thought someone might mention this but I do it when the form information is posted from the form (I use a clean up script to trim escape_string etc etc) so I shouldn’t need to do it again (I would hope).

[chilli11]

Thank you oddz. I have been looking into ActiveRecord and the likes of Cake, CodeIgnitor etc all day and get the idea but I have a couple of questions:

1. Is ActiveRecord automatically avialable or do I need to install it (have been looking at php.activerecord website) and do I have to run it on Ruby/Rails?
2. Can I just use ActiveRecord or do I need to use a framework as well (or is this just what I use to make my own)?
3. Would I be better using something like Cake or CodeIgnitor etc?

I look forward to the advise as I feel I have a bit of information overload from all my research today and am now a little confused as to the best approach and requirements.

[oddz]

@chilli11

Is ActiveRecord automatically avialable or do I need to install it (have been looking at php.activerecord website) and do I have to run it on Ruby/Rails?

ActiveRecord is not available by default. You would either need to build your own or use one of open source ones modeled based on the ActiveRecord pattern such as; propel, doctrine, etc.

@chilli11

Can I just use ActiveRecord or do I need to use a framework as well (or is this just what I use to make my own)?

No framework is necessary.

@chilli11

Would I be better using something like Cake or CodeIgnitor etc?

Maybe, maybe not. It depends on the end goal. If the end goal to push out some projects quickly than a Framework can aid that goal. If the goal is to learn everything you can about building sites or applications, than rolling your own is equally beneficial.

[chilli11]

Thank you again oddz - that has cleared things up.

As I have been coding for a good few years now (the reason for this thread is that I have become complacent with my coding and not kept up with improvements and progression just re-using and adjusting my classes and functions etc per project) so I don’t need the learning curve (apart from learning how to use an framework if I choose to) so it may be worthwhile for me to use one as I am currently re-developing a website for my own company and time is of the essence.

Thank you again - you have clarified things for me.

[Lemon_Juice]

@chilli11

Thank you for the information - I thought someone might mention this but I do it when the form information is posted from the form (I use a clean up script to trim escape_string etc etc) so I shouldn’t need to do it again (I would hope).

You will make your code cleaner if you escape values for db where you actually construct sql - in this case the Dogs class that you have posted here and not where you use the Dogs class. This way the class will become more reusable and independed from its calling code. Suppose you want to switch from db storage to plain file storage in the Dogs class - the calling code doesn’t have to know that you have made the switch, you only need to change Dogs. But if you mysql escape data before passing them to Dogs then you have a problem because text files don’t need mysql escape, so you either have to unescape or remove mysql_escape_string from all the places you use Dogs. Or, if you want to switch to another db, then mysql escape is inadequete, because you might need postgresql escape, for example. You may never need to do these changes but if you have them in mind your code will be better organized.

When you look at any implementation that deals with the db and abstracts sql (Propel, Doctrine, etc.), you never have or should escape anything in the code you write - the underlying system does it for you. Since your Dogs class abstracts sql (because you don’t pass any sql to it, only data), then the same rule should apply. As a side effect it relieves you from having to remember about escaping values outside this class and makes your code leaner and cleaner.

[lastcraft]

Hi…

@chilli11

I am just wondering if my usual way of creating classes can be simplified as I always seem to be repeating a lot of code and db field names in both the class and the page where it is used??

An snippet from one of my current classes:

Wow, you’re not kidding. The obvious solution, place all those fields in an array, is the correct one. From there you’ll be able to remove pretty much all of the duplication.

You can still have them look like real members of the object like this…

class Dog {
    private $row;

    ...
    function __get($name) {
        return $this->row[$name];
    }
}

You really want to read the book “Refactoring” by Martin Fowler. It’s pretty much the first step in becoming an OO programmer, or even any other type of programmer.

There are other fixes you could make to the code. You want me to make them?

yours, Marcus