To register for an Internet.com membership to receive newsletters and white papers, use the Register button ABOVE.
To participate in the message forums BELOW, click here
PHPBuilder.com  
 

 

Go Back   PHPBuilder.com > PHP Help > Code Critique

Code Critique Having someone critique your code is always a great way to hone the skills. Stop in and post your code to see what your peers may have done differently.

Reply
 
Thread Tools Rate Thread Display Modes
Old 02-04-2004, 09:18 AM   #1
bubblenut
Copyleft Commie
 
bubblenut's Avatar
 
Join Date: Oct 2003
Location: London
Posts: 2,370
Paginator Class

PHP Code:
<?php
class Paginator
{
    var
$IN_LIST=5;
    
    var
$page;
    var
$num_pages;
    var
$totrows;
    var
$perpage;
    var
$links=false;
    var
$limit=false;

    function
Paginator($page,$totrows,$perpage)
    {
        
$num_pages=(int)($totrows/$perpage)+($totrows%$perpage==0?0:1);
        if(
$page>$num_pages)
          
$this->page=1;
      else
          
$this->page=$page;
        
$this->num_pages=$num_pages;
        
$this->totrows=$totrows;
        
$this->perpage=$perpage;
    }
    
    function
getLinks($link)
    {
        if(
$this->links===false) {
            if(
$this->page>1) {
                
$links='<a href="'.$link.'page='.($this->page-1)."\">Prev</a> ";
            } else {
                
$links='Prev ';
            }
            if((
$this->page-((int)($this->IN_LIST/2)))<1) {
                
$start=1;
            } else {
                
$start=$this->page-((int)($this->IN_LIST/2));
            }
            if((
$start+$this->IN_LIST)>$this->num_pages) {
                
$start-=($start+$this->IN_LIST)-($this->num_pages+1);
                
$end=$this->num_pages;
            } else {
                
$end=($start+$this->IN_LIST)-1;
            }
            if(
$start>1) $links.="... ";
            for(
$i=$start;$i<=$end;$i++) {
                if(
$i==$this->page) {
                    
$links.=$i." ";
                } else {
                    
$links.='<a href="'.$link.'page='.$i.'">'.$i."</a> ";
                }
            }
            if(
$end<$this->num_pages) $links.="... ";
            if(
$this->page<$this->num_pages) {
                
$links.='<a href="'.$link.'page='.($this->page+1)."\">Next</a> ";
            } else {
                
$links.='Next ';
            }
            
$this->links=$links;
        }
        return
$this->links;
    }
    
    function
getLimit()
    {
        if(
$this->limit===false) {
            
$this->limit=(($this->page-1)*$this->perpage).",".$this->perpage;
        }
        return
$this->limit;
    }

    function
getPage()
    {
        return
$this->page;
    }

    function
getNumPages()
    {
        return
$this->num_pages;
    }

    function
getTotalRows()
    {
        return
$this->totrows;
    }

    function
getPerPage()
    {
        return
$this->per_page;
    }
}
?>
What do you think?
Bubble
__________________
If at first it doesn't work, slap it with a dirty hack.

Moral of the week: Never let a moral of the week go on for more than a week, it's even sillier than feeding the admins.

My Blog
bubblenut is offline   Reply With Quote
Old 02-04-2004, 05:32 PM   #2
planetsim
code | beer > sleep
 
Join Date: Sep 2002
Location: aus
Posts: 4,826
Havent been able to test it yet, it doesnt look too bad, but why the small functions

PHP Code:
function getPage()
    {
        return
$this->page;
    }

    function
getNumPages()
    {
        return
$this->num_pages;
    }

    function
getTotalRows()
    {
        return
$this->totrows;
    }

    function
getPerPage()
    {
        return
$this->per_page;
    }
They have been defined in the class, why not just use them from outside the class instead of making a function to just return them.

Other than that it doesnt look bad at all.
__________________
Dont be lazy Search
And use the Manual
Webmobo - Open Source News Script | Portfolio / Blog | Against TCPA
planetsim is offline   Reply With Quote
Old 02-05-2004, 05:30 AM   #3
bubblenut
Copyleft Commie
 
bubblenut's Avatar
 
Join Date: Oct 2003
Location: London
Posts: 2,370
Re small functions:
Just how I was tought to do it. Makes a bit more sense in Java, I'd make all the class variables private and then access is only allowed to them through the relevant function making it imposible to update it accept through the proper means.

I have found out that it doesn't work if the number of pages is less than the IN_LIST variable. I made a change to it last night to cover this but I can;t quite remember it (and it's at home) If I get some time later I'll try and sort it out.
Bubble
__________________
If at first it doesn't work, slap it with a dirty hack.

Moral of the week: Never let a moral of the week go on for more than a week, it's even sillier than feeding the admins.

My Blog
bubblenut is offline   Reply With Quote
Old 02-05-2004, 05:46 AM   #4
Weedpacket
Custom User Title™
 
Weedpacket's Avatar
 
Join Date: Aug 2002
Location: Rapid Offensive Unit "Foreign Object Damage"
Posts: 19,122
Quote:
Originally posted by bubblenut
Re small functions:
Just how I was tought to do it. Makes a bit more sense in Java, I'd make all the class variables private and then access is only allowed to them through the relevant function making it imposible to update it accept through the proper means.
For example, a complex number might be described in either Cartesian form (x+iy) or in polar form (rcis(&theta;)). Enforcing a get/set interface means you can have $cx_num->getX(), $cx_num->getY(), $cx_num->getR(), $cx_num->getTheta() and corresponding set methods without having to expose just which representation is being used internally by the class (maybe one, maybe the other, maybe neither, maybe both either synchronously or asynchronously - it doesn't matter!).


Just as an FYI, PHP will distinguish between the property $object->foo and the method $object->foo(). Saves on using "get" a lot. If you don't mind something of an abuse of power, $object->foo() could get the value of $object->foo, and $object->foo($bar) could set it. Whether that's a good idea or not though is debatable.

Also, with PHP5, you can declare properties to be private, and so can enforce the interface.
__________________
On two occasions I have been asked [by Members of Parliament], "Pray, Mr. Babbage, if you put into the machine wrong figures, will the right answers come out?" I am not able rightly to apprehend the kind of confusion of ideas that could provoke such a question.

Last edited by Weedpacket; 02-05-2004 at 05:55 AM.
Weedpacket is offline   Reply With Quote
Old 02-05-2004, 06:22 AM   #5
planetsim
code | beer > sleep
 
Join Date: Sep 2002
Location: aus
Posts: 4,826
Quote:
Originally posted by Weedpacket
Also, with PHP5, you can declare properties to be private, and so can enforce the interface.
But that will only start to be used widely when PHP 5 is stable its currently still a beta be a few more months cannot wait.
__________________
Dont be lazy Search
And use the Manual
Webmobo - Open Source News Script | Portfolio / Blog | Against TCPA
planetsim is offline   Reply With Quote
Old 02-05-2004, 05:08 PM   #6
Weedpacket
Custom User Title™
 
Weedpacket's Avatar
 
Join Date: Aug 2002
Location: Rapid Offensive Unit "Foreign Object Damage"
Posts: 19,122
Quote:
Originally posted by planetsim
But that will only start to be used widely when PHP 5 is stable its currently still a beta be a few more months cannot wait.
I didn't realise you were in such a rush.

Besides, if you write get/set interface methods now, upgrading to declare properties private in PHP5 would just be a matter of putting "private" declarations in the class definition. Not doing so, and accessing properties directly, would mean privatising involves going through all the scripts that use the class and digging out all the places where those properties are accessed.
__________________
On two occasions I have been asked [by Members of Parliament], "Pray, Mr. Babbage, if you put into the machine wrong figures, will the right answers come out?" I am not able rightly to apprehend the kind of confusion of ideas that could provoke such a question.

Last edited by Weedpacket; 02-05-2004 at 05:23 PM.
Weedpacket is offline   Reply With Quote
Old 02-05-2004, 06:02 PM   #7
bbaassiri
Senior Member
 
bbaassiri's Avatar
 
Join Date: Oct 2002
Location: canada
Posts: 165
haven't been able to run it but it looks ok, i had a hard time reading and where are those inline comments?

Remember this kiss approach can make your life simple and for others. (Keep It Simple Silly)

Heres a sniplet i used to calculate pagination stuff. I used by pages instead of number of records
PHP Code:
//initialise start values
$Paging                            = array();
$Paging['PageOffset']            = 0;
$Paging['NumOfRecords']            = 0;
$Paging['Limit']                = $Limit;
$Paging['NumOfPages']            = 0;
$Paging['isStart']                = 1;
$Paging['StartIndex']            = 0;
//determines if previous should be only displayed
$Paging['isLast']                = 1;

$Paging['NextPage']['Value']    = '';
$Paging['PrevPage']['Value']    = '';

//logic for setting up parameters to send to Data Layer
//Get vars are different then keys
//set the default sort if no sort is given, primarily sent by the column icon
$Paging['SortType']        = ( isset($PARAMS['Sort']) ) ?  $PARAMS['Sort'] : ASC;

//set the default order if no order is given, primarily sent by the column icon
$Paging['OrderBy']        = ( isset($PARAMS['Order']) ) ?  $PARAMS['Order'] : $DEFAULT_ORDER_BY;


//always start at zero if and only if an offset is not supplied
$Paging['PageOffset']    = ( !empty($PARAMS['Offset']) ) ? $PARAMS['Offset'] : 0 ;
bbaassiri is offline   Reply With Quote
Old 02-05-2004, 06:09 PM   #8
planetsim
code | beer > sleep
 
Join Date: Sep 2002
Location: aus
Posts: 4,826
Yup me very impatient.

Also yes correct there about private, Public etc.

You may also want added the new Object methods like construct, deconstruct , autoload.

Im more or less waiting for this better XML Support ive heard its good but im also hearing its getting improved.
__________________
Dont be lazy Search
And use the Manual
Webmobo - Open Source News Script | Portfolio / Blog | Against TCPA
planetsim is offline   Reply With Quote
Reply

Bookmarks


Currently Active Users Viewing This Thread: 1 (0 members and 1 guests)
 
Thread Tools
Display Modes Rate This Thread
Rate This Thread:

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is Off
HTML code is Off
Forum Jump


All times are GMT -4. The time now is 12:18 PM.






Acceptable Use Policy

internet.comMediabistrojusttechjobs.comGraphics.com

WebMediaBrands Corporate Info


Advertise | Newsletters | Feedback | Submit News

Legal Notices | Licensing | Permissions | Privacy Policy


Powered by vBulletin® Version 3.7.2
Copyright ©2000 - 2009, Jelsoft Enterprises Ltd.