Page 1 of 1

a performance related question

Posted: Thu Jan 04, 2007 3:27 am
by raghavan20
i have an adapter class for ISBN converter like the one below.

Code: Select all

//adapter converter class
class ISBNConverter{
	public static function convertISBN( $value, $to13=TRUE, $to10=FALSE ){

		//initialize 
			$IsbnConverter = new ISBN();
			$value = preg_replace( "#[^0-9a-zA-Z]#", "", $value );
			$result = NULL;
		
		//convert to 13
			if( $to13 === TRUE ){
				$result = $IsbnConverter->convert( $value );
			}
		
		//convert to 10
			if( $to10 === TRUE ){
				$temp = substr( $value, 3, 9 );
				$checksum10 = $IsbnConverter->genchksum10( $temp );
				if( $checksum10 == 10 ) $checksum10 = 'X';
				$result = $temp . $checksum10;
			}
			
		//return
			##echo $result;
			return $result;
		
	}
}
main isbn class

Code: Select all

<?php


class ISBN {

	//...more functions
	
}



?>
but the problem is, i instantiate ISBN class in the adapter class code and i guess that might create more objects as this conversion is used very often. now, i have come up with another design which will have a singleton in adpater so that only one ISBN instance is created at any point of time.

Code: Select all

<?php

//adapter for ISBN class
class ISBNConverter{

	private static $IsbnConverter;
	
	public static function getISBNInstance(){
	
		if( get_class( ISBNConverter::$IsbnConverter ) != "ISBN" ){
			ISBNConverter::$IsbnConverter = new ISBN();
		}
		
		return ISBNConverter::$IsbnConverter;
	}

	public static function convertISBN( $value, $to13=TRUE, $to10=FALSE ){

		//initialize 
			$IsbnConverter = ISBNConverter::getISBNInstance();
			$value = preg_replace( "#[^0-9a-zA-Z]#", "", $value );
			$result = NULL;
		
		//convert to 13
			if( $to13 === TRUE ){
				$result = $IsbnConverter->convert( $value );
			}
		
		//convert to 10
			if( $to10 === TRUE ){
				$temp = substr( $value, 3, 9 );
				$checksum10 = $IsbnConverter->genchksum10( $temp );
				if( $checksum10 == 10 ) $checksum10 = 'X';
				$result = $temp . $checksum10;
			}
			
		//return
			##echo $result;
			return $result;
		
	}
}


//main ISBN class
class ISBN {

//...many functions
	
}



?>

i would request your opinions about this design. please let me know if you can think of other alternatives. thanks.

Posted: Thu Jan 04, 2007 4:32 am
by raghavan20
i do see a problem even in my second design.
when i actually ran the code. i actually see the ISBN object is not stored in ISBNConverter::IsbnConverter all the time. every time, a request is made, the ISBN object is not available and reinstantiated again. any ideas?

Posted: Thu Jan 04, 2007 3:31 pm
by Christopher
I think I might do something like:

Code: Select all

class ISBN{
     function ISBN($value) {
          $this->value = $this->_init($value);
     }
     function _init($value) {
                //initialize
                eturn preg_replace( "#[^0-9a-zA-Z]#", "", $value );
               
     }
     function to10() {
                //convert to 13
                eturn $IsbnConverter->convert( $value );
    }
     function to13() {
                //convert to 10
                $temp = substr( $this->value, 3, 9 );
                $checksum10 = $IsbnConverter->genchksum10( $temp );
                if( $checksum10 == 10 ) $checksum10 = 'X';
                return $temp . $checksum10;

     }
}

Posted: Thu Jan 04, 2007 7:36 pm
by Begby
raghavan20 wrote:i do see a problem even in my second design.
when i actually ran the code. i actually see the ISBN object is not stored in ISBNConverter::IsbnConverter all the time. every time, a request is made, the ISBN object is not available and reinstantiated again. any ideas?
Firstly use self::$var instead of classname::$var when referencing a static variable or method from within the same class. This way if you change the name of the class, or extend the class and override methods, you won't have problems. The only reason you would want to use a singleton like this is if the object is created many many times on a single page request.

Secondly PHP is stateless and your entire script runs and then exits on every single page request. So every time the page loads then yes, a new object will be made, it will not be created once and then sit around for all the other page requests to access. There are ways to save objects between page requests using sessions but don't do it unless you absolutely have to (like if its a complex object with a bunch of data that you don't want to recreate from arrays). Typically serializing objects into a session is much slower than just recreating the object.

Thirdly, objects usually aren't all that costly in php anyways. If your object is making a database query on every load, or creating resources, then that is costly. But if its a huge object with 500 functions and two internal variables, recreating that object really isn't recreating 500 functions, its only recreating sorta kinda an array with two variables, so keep that in mind.

Lastly, don't get stuck pre-optimizing or analyzing the theoretical performance of your code, just fix the really obvious stuff. Well written OO code (and it looks like you are going down that path) lends itself well to later optimization. Concentrate on getting the job done then go back and fix your bottlenecks.

Posted: Fri Jan 05, 2007 5:13 am
by Ollie Saunders
Firstly use self::$var instead of classname::$var when referencing a static variable or method from within the same class.
arborint writes php 4 so he can't.
Lastly, don't get stuck pre-optimizing or analyzing the theoretical performance of your code, just fix the really obvious stuff. Well written OO code (and it looks like you are going down that path) lends itself well to later optimization. Concentrate on getting the job done then go back and fix your bottlenecks.
Amen! If only more people would.

Posted: Fri Jan 05, 2007 7:44 am
by Begby
ole wrote:arborint writes php 4 so he can't.
I was referring to the original poster who is using php 5.

Posted: Fri Jan 05, 2007 2:35 pm
by Christopher
ole wrote:arborint writes php 4 so he can't.
Actually I run PHP5 on my servers, I just try to post code in these forums that runs under PHP4 so more people can try it. You would use self:: but I would be leery of using a static global unless you really need it. I would try to use a real object first.

Posted: Fri Jan 05, 2007 3:43 pm
by raghavan20
Begby wrote: Firstly use self::$var instead of classname::$var when referencing a static variable or method from within the same class. This way if you change the name of the class, or extend the class and override methods, you won't have problems. The only reason you would want to use a singleton like this is if the object is created many many times on a single page request.
i like the fact using self instead of class name seems more logical. yeah, i was quite sure that i need a singleton.
Begby wrote: Secondly PHP is stateless and your entire script runs and then exits on every single page request. So every time the page loads then yes, a new object will be made, it will not be created once and then sit around for all the other page requests to access. There are ways to save objects between page requests using sessions but don't do it unless you absolutely have to (like if its a complex object with a bunch of data that you don't want to recreate from arrays). Typically serializing objects into a session is much slower than just recreating the object.
i was not aware earlier that gc is run after every page request is done and i thought objects created stay in the memory and i found that what you said is true through debugging. thanks for the tip that serializing objects into session is expensive than recreating the object itself.
Begby wrote: Thirdly, objects usually aren't all that costly in php anyways. If your object is making a database query on every load, or creating resources, then that is costly. But if its a huge object with 500 functions and two internal variables, recreating that object really isn't recreating 500 functions, its only recreating sorta kinda an array with two variables, so keep that in mind.
yeah, that is true
Begby wrote: Lastly, don't get stuck pre-optimizing or analyzing the theoretical performance of your code, just fix the really obvious stuff. Well written OO code (and it looks like you are going down that path) lends itself well to later optimization. Concentrate on getting the job done then go back and fix your bottlenecks.
what you are saying is right but i have already done with the functionality and i saw that one object may be created may times unnecessarily.

Posted: Fri Jan 05, 2007 4:27 pm
by Christopher
raghavan20 wrote: what you are saying is right but i have already done with the functionality and i saw that one object may be created may times unnecessarily.
This does not seem like much of a object unless you are using it as such -- retaining internal state. I think I would just make this a helper function the way you are using it.

Posted: Fri Jan 05, 2007 4:37 pm
by raghavan20
arborint wrote:
raghavan20 wrote: what you are saying is right but i have already done with the functionality and i saw that one object may be created may times unnecessarily.
This does not seem like much of a object unless you are using it as such -- retaining internal state. I think I would just make this a helper function the way you are using it.
this is what i was also thinking of, just making all the methods in the ISBN class as static because the ISBN class I found in the internet does not actually hold any attributes. it just makes calls to other fellow class functions.


For more clarity, i have posted the original ISBN class.

main isbn class

Code: Select all

<?php
/*
* @Version 1.0
* @Author John F. Blyberg <blybergj@aadl.org> - http://www.blyberg.net
* @Desc ISBN Class, adapted from ISBN.pm - http://www.manasystems.co.uk/isbnpm.html
*/

class ISBN {


	public function convert($isbn) {
		$isbn2 = substr("978" . trim($isbn), 0, -1);
		$sum13 = self::genchksum13($isbn2);
		$isbn13 = $isbn2.$sum13;
		return ($isbn13);
	}

	public function gettype($isbn) {
		$isbn = trim($isbn);
		if (preg_match('%[0-9]{12}?[0-9Xx]%s', $isbn)) {
			return 13;
		} else if (preg_match('%[0-9]{9}?[0-9Xx]%s', $isbn)) {
			return 10;
		} else {
			return -1;
		}
	}

	public function validateten($isbn) {
		$isbn = trim($isbn);
		$chksum = substr($isbn, -1, 1);
		$isbn = substr($isbn, 0, -1);
		if (preg_match('/X/i', $chksum)) { $chksum="10"; }
		$sum = &self::genchksum10($isbn);
		if ($chksum == $sum){
			return 1;
		}else{
			return 0;
		}
	}

	public function validatettn($isbn) {
		$isbn = trim($isbn);
		$chksum = substr($isbn, -1, 1);
		$isbn = substr($isbn, 0, -1);
		if (preg_match('/X/i', $chksum)) { $chksum="10"; }
		$sum = self::genchksum13($isbn);
		if ($chksum == $sum){
			return 1;
		}else{
			return 0;
		}
	}

	public function genchksum13($isbn) {
		$isbn = trim($isbn);
		$tb = 0;
		for ($i = 0; $i <= 12; $i++) {
			$tc = substr($isbn, -1, 1);
			$isbn = substr($isbn, 0, -1);
			$ta = ($tc*3);
			$tci = substr($isbn, -1, 1);
			$isbn = substr($isbn, 0, -1);
			$tb = $tb + $ta + $tci;
		}
		$tg = ($tb / 10);
		$tint = intval($tg);
		if ($tint == $tg) { return 0; }
		$ts = substr($tg, -1, 1);
		$tsum = (10 - $ts);
		return $tsum;
	}

	public function genchksum10($isbn) {
		$t = 2;
		$isbn = trim($isbn);
		$a = NULL; $b = NULL;
		for($i = 0; $i <= 9; $i++){
			$b = $b + $a;
			$c = substr($isbn, -1, 1);
			$isbn = substr($isbn, 0, -1);
			$a = ($c * $t);
			$t++;
		}
		$s = ($b / 11);
		$s = intval($s);
		$s++;
		$g = ($s * 11);
		$sum = ($g - $b); 
		return $sum;
	}

	public function printinvalid() {
		print "That is an invalid ISBN number";
		exit;
	}
	
}



?>

Posted: Fri Jan 05, 2007 7:58 pm
by Christopher
That looks list a very helpful class. It seems like the main use cases are to determine the type, validate ISBN10 and ISBN13 codes, and to convert from 10 to 13. That's the first four methods. The other methods might be private or be named starting with underscores to indicate that the are support functions. I might spell out and camelcase the method names and maybe try to make them a little clearer (e.g. 10to13() instead of convert()). Oh, and remove the printinvalid() method because it does not really belong.

Posted: Sat Jan 06, 2007 5:10 am
by raghavan20
arborint wrote:That looks list a very helpful class. It seems like the main use cases are to determine the type, validate ISBN10 and ISBN13 codes, and to convert from 10 to 13. That's the first four methods. The other methods might be private or be named starting with underscores to indicate that the are support functions. I might spell out and camelcase the method names and maybe try to make them a little clearer (e.g. 10to13() instead of convert()). Oh, and remove the printinvalid() method because it does not really belong.
for your info, i did not write that class and i am not really interested in making that class better. i am just making an adapter for this class so that all code inside my application can use the adapter interface. as one can see the class is just helpful but it does not use any attribute and anybody can say that it really works like a normal set of functions and it is just grouped under the class ISBN.


thanks for all your comments guys.