What is/is not considered to be good OO programming
Tony Marston
http://www.tonymarston.net/php-mysql/databaseobjects.html which
describes an abstract class I have written to control all access to my
databases. Someone who is supposed to be a respected member of the PHP
community has informed me that my class breaks the rules of OO
programming and is therefore unworthy of serious consideration. To be
specific he complained that my class contains the following
variables:-
$rows_per_page (to set the page size)
$page_no (to request a particular page number)
$lastpage (returns the highest possible page number)
which he considers to related to formatting rather than the data
itself, and as such is *bad* programming.
I personally think that he is talking out of the wrong end of his
alimentary canal for the following reasons:-
1) Those variables are not for *formatting*, they are for *data
selection*. When my presentation layer communicates with a database
object it is basically saying "get me the data that satisfies this
selection criteria" part of which is "based on a page size of
$row_per_page get me the records for page $page_no, and tell me the
highest possible page available".
2) Despite me asking him what he considers to be the *right* way of
doing things he has failed to respond, probably because he doesn't
know the answer.
3) There is nothing in any OOP literature I have read that says what I
am doing is wrong, there it is not wrong.
4) The language allows me to do it, therefore it cannot be wrong.
5) It is simple and it works, therefore it cannot be all that bad.
Do you people out there in PHP-land have any opinions on this matter?
Is this critocism justified or not?
Tony Marston
http://www.tonymarston.net/
Martin Wickman
> On my website I have an article
> http://www.tonymarston.net/php-mysql/databaseobjects.html which
> describes an abstract class I have written to control all access to
> my databases. Someone who is supposed to be a respected member of
> the PHP community has informed me that my class breaks the rules of
> OO programming and is therefore unworthy of serious consideration.
> he considers [the code to be ] related to formatting rather than the
> data itself, and as such is *bad* programming.
Everyone thinks they know what correct OO is, unfortunately I am the
only one that truly knows it. So here is the correct response:
Gimme a break. There is _nothing_ in OO which says that you should not
mix presentation code with logic code. OO deals with encapsulation and
dependencies, not implementation. As long concerns are separated "well
enough", there is no reason to worry.
Otoh, there is general consensus regarding GUI-apps which says you
should not mix presentation with logic. But that is more like common
knowledge than OO.
> 2) Despite me asking him what he considers to be the *right* way of
> doing things he has failed to respond, probably because he doesn't
> know the answer.
Probably.
> 4) The language allows me to do it, therefore it cannot be wrong.
Yup, otherwise you'd get a syntax error...
> 5) It is simple and it works, therefore it cannot be all that bad.
Yes, that is the single most important point to make:
KISS -- keep it simple, stupid.
Anything else will just make it a lot harder to keep up when the
system demands (inevitable) change.
> Do you people out there in PHP-land have any opinions on this matter?
> Is this critocism justified or not?
I think the OO features in PHP3/4 sucks. I mostly use PHPs object as
simple wrappers for variables (structs) and keep 95% of all
programming functional.
Dave Martin
to...@marston-home.demon.co.uk says...
> http://www.tonymarston.net/php-mysql/databaseobjects.html which
> describes an abstract class I have written to control all access to my
> databases.
Nicely done Tony.
> 1) Those variables are not for *formatting*, they are for *data
> selection*. When my presentation layer communicates with a database
> object it is basically saying "get me the data that satisfies this
> selection criteria" part of which is "based on a page size of
> $row_per_page get me the records for page $page_no, and tell me the
> highest possible page available".
Well, in a way, they are for formatting really. Your layer doesn't need
any concept of a page per se since it really has nothing to do with
actually doing anything with the data.
> 3) There is nothing in any OOP literature I have read that says what I
> am doing is wrong, there it is not wrong.
I certainly wouldn't call it "wrong" that's for sure. You've simply
added a small bit of presentation logic to your class.
> Is this critocism justified or not?
I'd say not. If you took what you have and moved the
"formatting/presentation" bits into a separate class leaving just the
data manipulation bits in your class it might squash that criticism.
Personally I'd probably do it exactly the way you have since it's rare
that data manipulation and presentation are totally separate things
(IMO).
David Martin
DynaComp Solutions
http://DynaComp-Solutions.com
Andy Jeffries
>> 4) The language allows me to do it, therefore it cannot be wrong.
>
> Yup, otherwise you'd get a syntax error...
I disagree, something can be "wrong" as in an ugly way of doing something
but by syntactically correct.
However, I agree there is nothing wrong with what he is doing.
Cheers,
Andy
stephan beal
> $rows_per_page (to set the page size)
> $page_no (to request a particular page number)
> $lastpage (returns the highest possible page number)
>
> which he considers to related to formatting rather than the data
> itself, and as such is *bad* programming.
i wouldn't go so far as to say BAD, but i would consider it to be ill-suited
to the problem. i agree with him entirely that it's strictly formatting
info, and therefor has no place in a db abstraction layer. It does,
however, have a place in a DbTableRenderer, e.g., or a layer which builds
off of the db layer.
> 1) Those variables are not for *formatting*, they are for *data
> selection*.
No, they're not: consider that a PAGE is a layout convention, and layout is
formatting. Data selection has no unambigious concept of "page".
> 3) There is nothing in any OOP literature I have read that says what I
> am doing is wrong, there it is not wrong.
Yes, there is: most OO advocates will tell you to avoid ANY use of public
variables, for example.
> 4) The language allows me to do it, therefore it cannot be wrong.
The English language allows me to say many abusive things to you, but would
i be right in doing so? Languages are expressive tools, it's the
/expression/ of your solution which is not optimal: a) no public vars and
b) separate formatting from data.
> 5) It is simple and it works, therefore it cannot be all that bad.
If it serves the purpose, great, and that makes it good for it's purpose.
However, that almost inherently makes it less flexible for later adaption
into other contexts.
just my opinions, of course...
--
----- stephan beal
Registered Linux User #71917 http://counter.li.org
I speak for myself, not my employer. Contents may
be hot. Slippery when wet. Reading disclaimers makes
you go blind. Writing them is worse. You have been Warned.
matty
<snip>
> $rows_per_page (to set the page size)
> $page_no (to request a particular page number)
> $lastpage (returns the highest possible page number)
>
> which he considers to related to formatting rather than the data
> itself, and as such is *bad* programming.
I'd agree, and say that he's using the output buffer of the canal.
I've got something similar, although I have a subclass for paged output,
but much the same kind of thing.
Apart from nicer presentation, paging the results allows you to reduce
communication with the database server; if you insist on returning the
entire result set from the query, and *then* choosing a subset of it
from the PHP code, you introduce an unnecessary layer, which can be
done away with entirely by using "LIMIT x, y" in the query.
I think you've done it the right way (but then I would, 'cos I have too!)
> 5) It is simple and it works, therefore it cannot be all that bad.
>
> Do you people out there in PHP-land have any opinions on this matter?
> Is this critocism justified or not?
Definitely unjustified. Is he a first-year S/W Eng student by any chance???
matty
using "LIMIT 0, 10", etc, in SQL?
Does he feel that it's bad, since the RDBMS is
taking part in presentation, which is wrong???
;-)
André Næss
> On my website I have an article
> http://www.tonymarston.net/php-mysql/databaseobjects.html which
> describes an abstract class I have written to control all access to my
> databases. Someone who is supposed to be a respected member of the PHP
> community has informed me that my class breaks the rules of OO
> programming and is therefore unworthy of serious consideration. To be
> specific he complained that my class contains the following
> variables:-
>
> $rows_per_page (to set the page size)
> $page_no (to request a particular page number)
> $lastpage (returns the highest possible page number)
>
> which he considers to related to formatting rather than the data
> itself, and as such is *bad* programming.
>
> I personally think that he is talking out of the wrong end of his
> alimentary canal for the following reasons:-
>
> 1) Those variables are not for *formatting*, they are for *data
> selection*. When my presentation layer communicates with a database
> object it is basically saying "get me the data that satisfies this
> selection criteria" part of which is "based on a page size of
> $row_per_page get me the records for page $page_no, and tell me the
> highest possible page available".
The information is typical formatting information. I think it would be much
cleaner if you allowed for two more arguments to getData() that defined how
many rows to retrieve, and what offset to start at. The organization of
data into pages belongs in presentation code.
From a purist perspective, SQL operates on sets, and so operations like "get
me item 1 through 10" makes no sense since sets have no intrinsic ordering.
But the second one applies the SORT operator, the result is no longer a
set, but a list -- a structure with a defined order, and hence the
retrieval of rows 1 through 10 suddenly makes sense.
> 3) There is nothing in any OOP literature I have read that says what I
> am doing is wrong, there it is not wrong.
Right or wrong here is a matter of degree. If one agree that the variables
have to do with formatting, then what you have done is bad OO practice
according to a lot of people. The point is that good design typically
requires that classes have responsibilities, and it's not natural for a
database abstraction layer to be responsible for formatting.
> 4) The language allows me to do it, therefore it cannot be wrong.
Again, it depends on your usage of the word wrong. C allows me to write code
that generates random segmentation faults, and assembler allows me to write
self-modifying code.
> 5) It is simple and it works, therefore it cannot be all that bad.
I agree that it's simple and it does the job. But as far as I could
understand you wanted to submit your code to some sort of repository. When
code is to be used by many people it obviously has to be of much higher
quality than most of the code that's written every day, and -- more
importantly -- it must adhere to the principles accepted by most of the
developers who are supposed to use the code.
And in general I think your design is bad because you have to create a new
class for every table you create, and you have to recode both the table and
the class every time you change the table, which obviously doubles the
chances of bugs. I can't really see how this can save you very much work I
guess.
André Næss
Jochen Daum
On 7 Aug 2003 04:45:59 -0700, to...@marston-home.demon.co.uk (Tony
Marston) wrote:
>On my website I have an article
>http://www.tonymarston.net/php-mysql/databaseobjects.html which
>describes an abstract class I have written to control all access to my
>databases. Someone who is supposed to be a respected member of the PHP
>community has informed me that my class breaks the rules of OO
>programming and is therefore unworthy of serious consideration. To be
>specific he complained that my class contains the following
>variables:-
>
>$rows_per_page (to set the page size)
>$page_no (to request a particular page number)
>$lastpage (returns the highest possible page number)
>
>which he considers to related to formatting rather than the data
>itself, and as such is *bad* programming.
>
The others wrote everything I could write.
My database class also supports a limit statement, I especially wrote
it, because its such a hassle to limit with MSSQL.
IMO, doesn't matter, if irt is parameter or class variable. I prefer
parameter, because I don't like:
$arith->x = 1;
$arith->y = 2;
$m = arith->multiply();
I rather like:
$m = arith->multiply(1,2);
HTH, Jochen
>I personally think that he is talking out of the wrong end of his
>alimentary canal for the following reasons:-
>
>1) Those variables are not for *formatting*, they are for *data
>selection*. When my presentation layer communicates with a database
>object it is basically saying "get me the data that satisfies this
>selection criteria" part of which is "based on a page size of
>$row_per_page get me the records for page $page_no, and tell me the
>highest possible page available".
>
>2) Despite me asking him what he considers to be the *right* way of
>doing things he has failed to respond, probably because he doesn't
>know the answer.
>
>3) There is nothing in any OOP literature I have read that says what I
>am doing is wrong, there it is not wrong.
>
>4) The language allows me to do it, therefore it cannot be wrong.
>
>5) It is simple and it works, therefore it cannot be all that bad.
>
>Do you people out there in PHP-land have any opinions on this matter?
>Is this critocism justified or not?
>
>Tony Marston
>http://www.tonymarston.net/
--
Jochen Daum - CANS Ltd.
PHP DB Edit Toolkit -- PHP scripts for building
database editing interfaces.
http://sourceforge.net/projects/phpdbedittk/
matty
<snip>
> The information is typical formatting information. I think it would be
> much cleaner if you allowed for two more arguments to getData() that
> defined how many rows to retrieve, and what offset to start at. The
> organization of data into pages belongs in presentation code.
The information is used to select a subset of the data, so how is this
formatting? Just because this kind of technique is commonly used in
presentation, doesn't mean that restricting the rows returning is a
formatting operation. If you were to take that viewpoint, then you
could argue that since the order records are displayed in a table is
presentational, then ordering the records returned is also a formatting
operation.
I do think that a GetPagedChunk($offset, $length) method could be a good
idea; however, the concepts of orthogonality and encapsulation would
indicate that having created a PagedResultSet($sourcequery, $pagelimit, $offset=0)
object (or similar), really, the object should handle issues such as which
records to return.
This *can* verge into formatting, but it's a trciky question: the presentational
requirements of displaying a table of data, along with "Showing records 5-19 of 235",
generation of next/previous links, etc, leads to a very heterogeneous set of data
to be returned by the object. One possible way to handle this is to have
GetPageNumber(), GetPageCount(), GetNextOffset(), etc, methods; another is to
return data as xml for transformation into the desired *format* for display, since
a generalized class cannot "know" the format required, etc.
>
> From a purist perspective, SQL operates on sets, and so operations like
> "get me item 1 through 10" makes no sense since sets have no intrinsic
> ordering. But the second one applies the SORT operator, the result is no
> longer a set, but a list -- a structure with a defined order, and hence
> the retrieval of rows 1 through 10 suddenly makes sense.
Arguing whether this is a part of a true relational query or not is fairly
academic as far as developing an application goes: very few large-scale applications
are built using databases in 5NF, since performance issues take a part. Sets have
no intrinsic ordering, but most people writing this type of class apply some
kind of ordering to the set returned, so that taking "records 16-20 of 33" has
some kind of meaningful semantic.
> Right or wrong here is a matter of degree. If one agree that the variables
> have to do with formatting, then what you have done is bad OO practice
> according to a lot of people. The point is that good design typically
> requires that classes have responsibilities, and it's not natural for a
> database abstraction layer to be responsible for formatting.
Good OO is about code reuse, orthogonality, and data encapsulation. Since the
data access class is there to handle the database queries, wouldn't it be
breaking the encapsulation to have a separate entity control which data is
actually displayed?
One of the funny things about this kind of application is that selecting
a subset of the data sits somewhere between database handling and presentation.
Reducing the number of records displayed reduces the overhead on 1) user
screens (and user brains); and on 2) the system serving the pages. I still
think there is a strong case that this kind of class doesn't have to violate
principles of OO; even if it did, the issues of performance dictate that it's
better to have the class function in this way, to increase code reliability and
reuse. People use OO to produce more reliable software, more rapidly: just
because a system is OO doesn't make it good in itself, it merely means it
has been written to conform to a specific paradigm.
> I agree that it's simple and it does the job. But as far as I could
> understand you wanted to submit your code to some sort of repository. When
> code is to be used by many people it obviously has to be of much higher
> quality than most of the code that's written every day, and -- more
> importantly -- it must adhere to the principles accepted by most of the
> developers who are supposed to use the code.
>
> And in general I think your design is bad because you have to create a new
> class for every table you create, and you have to recode both the table
> and the class every time you change the table, which obviously doubles the
> chances of bugs. I can't really see how this can save you very much work I
> guess.
Why do you have to recode the class? Admittedly, in the tutorial mentioned, the
number of rows per page is set in the constructor; there is no reason why the
variable cannot be changed. The same applies to the table name and database name.
As far as code quality is concerned, there are things missing (escaping the data
for one). But from a reusability point of view, mostly all that needs changing
is to pass a few parameters to the constructor, and add sorting to the queries
to get consistent results.
I think, though, that the page was meant more as a tutorial on writing classes
in PHP, and as such is better than many of those around online to date.
Zurab Davitiani
> Tony Marston wrote:
>> 1) Those variables are not for *formatting*, they are for *data
>> selection*.
>
> No, they're not: consider that a PAGE is a layout convention, and layout
> is formatting. Data selection has no unambigious concept of "page".
Paging the data is being used by most databases that feature variable-length
records, such as Access, MSSQL, and others. While Tony's implementation
does not exactly serve the same purpose, it still goes to show that "page"
is not necessarily related to a presentation layer.
It seems to me that the issue here is with the naming of the properties with
"page" in them, not the merits of the properties, methods and
functionalities they provide. I believe it is perfectly acceptable for a
data layer to be able to divide data into "chunks" and only return
requested appropriate chunk(s) to the calling object/component/service/etc.
>> 3) There is nothing in any OOP literature I have read that says what I
>> am doing is wrong, there it is not wrong.
>
> Yes, there is: most OO advocates will tell you to avoid ANY use of public
> variables, for example.
Yes, I spotted this too and you are absolutely right.
--
Business Web Solutions
ActiveLink, LLC
www.active-link.com/intranet/
matty
> Yes, there is: most OO advocates will tell you to avoid ANY use of public
> variables, for example.
>
Then you're screwed in PHP, since it doesn't support a top-level "application"
object; the closest you could get, is have an object that does everything from
its constuctor, and start it all off with
new MyApplication();
but that's probably at least a *little* counterintuitive...
Tony Marston
Rather have have extra arguments in my 'getData' method I have
separate setters for $rows_per_page and $pageno, with a getter for
$lastpage. The reason for this is that a default value for
$rows_per_page can be defined in the class constructor.
> From a purist perspective, SQL operates on sets, and so operations like "get
> me item 1 through 10" makes no sense since sets have no intrinsic ordering.
> But the second one applies the SORT operator, the result is no longer a
> set, but a list -- a structure with a defined order, and hence the
> retrieval of rows 1 through 10 suddenly makes sense.
My point is that the presentation layer asks the database object to
retrieve some data, and it is the database object is responsible for
constructing the SQL statement. As the SQL statement contains the
'LIMIT x,y' clause it is necessary for the database object to have
these values available. There is simply no other way.
> > 3) There is nothing in any OOP literature I have read that says what I
> > am doing is wrong, there it is not wrong.
>
> Right or wrong here is a matter of degree. If one agree that the variables
> have to do with formatting, then what you have done is bad OO practice
> according to a lot of people. The point is that good design typically
> requires that classes have responsibilities, and it's not natural for a
> database abstraction layer to be responsible for formatting.
Those variables are not used for *formatting* the data, they are used
for *selecting* a subset of rows instead of the entire contents of the
table which would be grossly inefficient.
> > 4) The language allows me to do it, therefore it cannot be wrong.
>
> Again, it depends on your usage of the word wrong. C allows me to write code
> that generates random segmentation faults, and assembler allows me to write
> self-modifying code.
I am aware that certain languages have functions which are more
trouble than they are worth (I remember the ALTER verb in COBOL) but
that is irrelevant in this case. I do not see why it could possibly be
*wrong* to have a variable in my database object which allows that
database object to construct the LIMIT clause in an sql SELECT
statement.
> > 5) It is simple and it works, therefore it cannot be all that bad.
>
> I agree that it's simple and it does the job. But as far as I could
> understand you wanted to submit your code to some sort of repository. When
> code is to be used by many people it obviously has to be of much higher
> quality than most of the code that's written every day, and -- more
> importantly -- it must adhere to the principles accepted by most of the
> developers who are supposed to use the code.
I have been a software developer for over 25 years and I have seen
many different programmers with many different principles. Some try to
produce code which is efficient, effective and maintainable then write
a methodology to match, while others create a fancy methodology
without any regard for efficiency or maintainability. As far as I am
concerned the overriding principle is the KISS principle (Keep It
Simple, Stupid). Too many people keep inventing stupid rules which
just get in the way of efficient software development. Too many people
have different ideas about what is *good* and *not good* practice. As
far as I am concerned if it works, is simple and is maintainable then
it is *good*. Anything else is *not good*.
> And in general I think your design is bad because you have to create a new
> class for every table you create, and you have to recode both the table and
> the class every time you change the table, which obviously doubles the
> chances of bugs. I can't really see how this can save you very much work I
> guess.
> André Næss
You have missed the point about my abstract database class. Each
database table has its own class which is a subclass of the abstract
class, therefore it inherits all the properties and methods of the
abstract class. Each table class therefore need only contain extra
code which is specific to that class, such as a list of field names,
validation rules, relationships, etc. All the code for retrieving,
inserting, updating, deleting and validating data is inherited from
the abstract class and does not have to be rewritten or copied.
Tony Marston
http://www.tonymarston.net/
Tony Marston
> In article <7588a50f.03080...@posting.google.com>,
> to...@marston-home.demon.co.uk says...
>
> > http://www.tonymarston.net/php-mysql/databaseobjects.html which
> > describes an abstract class I have written to control all access to my
> > databases.
>
> Nicely done Tony.
Thanks.
> > 1) Those variables are not for *formatting*, they are for *data
> > selection*. When my presentation layer communicates with a database
> > object it is basically saying "get me the data that satisfies this
> > selection criteria" part of which is "based on a page size of
> > $row_per_page get me the records for page $page_no, and tell me the
> > highest possible page available".
>
> Well, in a way, they are for formatting really. Your layer doesn't need
> any concept of a page per se since it really has nothing to do with
> actually doing anything with the data.
They are not *formatting* the data, they are used for *selecting* rows
from the database in the same way as 'where columnname=value' is used
for selection. The only *formatting* which is done is when the HTML
code is generated, which is done using an XML file and an XSL
transformation.
> > 3) There is nothing in any OOP literature I have read that says what I
> > am doing is wrong, there it is not wrong.
>
> I certainly wouldn't call it "wrong" that's for sure. You've simply
> added a small bit of presentation logic to your class.
>
> > Is this critocism justified or not?
>
> I'd say not. If you took what you have and moved the
> "formatting/presentation" bits into a separate class leaving just the
> data manipulation bits in your class it might squash that criticism.
>
> Personally I'd probably do it exactly the way you have since it's rare
> that data manipulation and presentation are totally separate things
> (IMO).
The database object needs to construct the sql SELECT statement, which
contains the LIMIT clause. It is therefore imperative that the
database object has the information available to construct the LIMIT
clause. There is simply no other way.
Tony Marston
http://www.tonymarston.net/
Tony Marston
> Tony Marston wrote:
> > $rows_per_page (to set the page size)
> > $page_no (to request a particular page number)
> > $lastpage (returns the highest possible page number)
> >
> > which he considers to related to formatting rather than the data
> > itself, and as such is *bad* programming.
>
> i wouldn't go so far as to say BAD, but i would consider it to be ill-suited
> to the problem. i agree with him entirely that it's strictly formatting
> info, and therefor has no place in a db abstraction layer. It does,
> however, have a place in a DbTableRenderer, e.g., or a layer which builds
> off of the db layer.
>
> > 1) Those variables are not for *formatting*, they are for *data
> > selection*.
>
> No, they're not: consider that a PAGE is a layout convention, and layout is
> formatting. Data selection has no unambigious concept of "page".
Those variables are for SELECTION as they are used to construct the
LIMIT clause on the sql SELECT statement. Even if the sql statement is
generated in a separate object these variables must be passed to that
object, therefore they must exist in my database class.
> > 3) There is nothing in any OOP literature I have read that says what I
> > am doing is wrong, there it is not wrong.
>
> Yes, there is: most OO advocates will tell you to avoid ANY use of public
> variables, for example.
I only use public variables because PHP 4 does not cater for private
variables. I am aware however that these variables should only be
accessed through separate setters and getters, which is what I use.
> > 4) The language allows me to do it, therefore it cannot be wrong.
>
> The English language allows me to say many abusive things to you, but would
> i be right in doing so? Languages are expressive tools, it's the
> /expression/ of your solution which is not optimal: a) no public vars and
> b) separate formatting from data.
People who say that my method is *wrong* have yet to supply me their
version of a method which is *right*.
> > 5) It is simple and it works, therefore it cannot be all that bad.
>
> If it serves the purpose, great, and that makes it good for it's purpose.
> However, that almost inherently makes it less flexible for later adaption
> into other contexts.
>
>
> just my opinions, of course...
How can my method be inflexible?
stephan beal
> stephan beal <ste...@wanderinghorse.net> wrote in message
>> Yes, there is: most OO advocates will tell you to avoid ANY use of public
>> variables, for example.
>
> I only use public variables because PHP 4 does not cater for private
> variables. I am aware however that these variables should only be
> accessed through separate setters and getters, which is what I use.
Then don't publish the vars in your API and add functions to get at them
instead. Yes, it's not the most efficient solution, but it's simple to
implement and makes long-term maintenance simpler.
>> > 5) It is simple and it works, therefore it cannot be all that bad.
>>
>> If it serves the purpose, great, and that makes it good for it's purpose.
>> However, that almost inherently makes it less flexible for later adaption
>> into other contexts.
>>
>>
>> just my opinions, of course...
>
> How can my method be inflexible?
Let's say it's 6 months from now and your needs for your site have out-grown
what the framework of capable of. The question comes up: rewrite the site
or expand the existing framework? The more that a framework (i.e., your
database layer) is tied to a specific task, the less flexible it becomes
and the more difficult it becomes to re-use that code in other contexts.
i'm not at all saying that your db layer IS inflexible (i have no idea - i
haven't analysed it), but i am of the opinion that SELECT data, for
example, is in essence a formatting option (the WHERE clause, on the other
hand, is specifically a logic operation), and that shoe-horning such things
as paging into the db layer isn't necessary. i would first write the db
layer and then make another layer which includes the selection/limitation
code. What if i wanted to use your db layer but didn't want the paging
code? Is the class designed in such a way that i am forced to use it?
Just things to consider.
Glen Vermeylen
Tables are part of a database, your approach only leaves room to work
with one database. If you create a database class (as in: $db = new
dataBase('server', 'username', 'password') you could have your table
class make use of this dataBase class as in $table = new
table($db->getLink()).
Then you can distanciate yourself from the whereabouts of the tables and
only worry about their data.
Considering your getData: this requires you to have insight into the
inner mechanics of your class (you have to set your desired pagesize,
and you have to go through trouble if you don't want to get the standard
subsequent pages, set by pagesize).
I suggest you have a method $table->getData(selection_criteria), which
gives you every result there is. Then you could have a method
$table->getPage(selection_criteria, pagesize='defaultvalue',
pagenumber='1'). (the "=" will use a defaultvalue, if you don't pass
that parameter).
Now you can make a getNextPage(), getPrevPage(), ... which do what you
can guess... :).
I don't know anything about error-handling with php, but you might want
to consider tying mysql-specific errors to your mysql-classes.
$results = $table->getPage(...);
if ($table->hasError())
$table->printError();
That way, with inheritance (and overriding) your $db errors can be
delegated to $table...
I hope this has been of any help.
ps: This is not written out of arrogance and I don't think I know it
better than anyone else. I'm just a student, and also was pondering
about a nice oo-approach to php/mysql scripting.
This is just a mere braindump.
Glen
Glen Vermeylen
"new table($db->getLink())" would offcourse be "new table($db)". The
user only knows that a table is part of a database, he shouldn't know
about the link.
"That way, with inheritance (and overriding) ... " should be "with
delegation ... "
:-)
André Næss
> André Næss wrote:
>
> <snip>
>
>> The information is typical formatting information. I think it would be
>> much cleaner if you allowed for two more arguments to getData() that
>> defined how many rows to retrieve, and what offset to start at. The
>> organization of data into pages belongs in presentation code.
>
> The information is used to select a subset of the data, so how is this
> formatting? Just because this kind of technique is commonly used in
> presentation, doesn't mean that restricting the rows returning is a
> formatting operation. If you were to take that viewpoint, then you
> could argue that since the order records are displayed in a table is
> presentational, then ordering the records returned is also a formatting
> operation.
Ok let me try to clarify this. To fetch rows 1 through 10 is ok. Using limit
is ok. It's ok to define what subset of a sorted result set you want,
because that's part of the selection. I agree.
Here's the relevant code:
var $rows_per_page; // used in pagination
var $pageno; // current page number
var $lastpage; // highest page number
So what Tony has done is put *pagination* information into the DB-class, and
that is what I react to. He could insert two variables, $offset and
$numRows, and that would be ok. But the pagination belongs in the
presentation layer. It's a tiny difference, and not very important IMO, but
I still think it's worth pointing out.
>> And in general I think your design is bad because you have to create a
>> new class for every table you create, and you have to recode both the
>> table and the class every time you change the table, which obviously
>> doubles the chances of bugs. I can't really see how this can save you
>> very much work I guess.
>
> Why do you have to recode the class? Admittedly, in the tutorial
> mentioned, the number of rows per page is set in the constructor; there is
> no reason why the
> variable cannot be changed. The same applies to the table name and
> database name.
You have to write a new class which inherits the abstract class and has
variables to represent the fields in the DB. Quote from Tony's page:
function Database_Table ()
{
$this->tablename = 'default';
$this->dbname = 'default';
$this->rows_per_page = 10;
$this->fieldlist = array('column1', 'column2', 'column3');
$this->fieldlist['column1'] = array('pkey' => 'y');
} // constructor
The variable $fieldlist is used to list all the columns within that table,
and to identify which is the primary key. How this is used will become
apparent
End quote.
So when you add a field to your DB, you also have to change the class, this
sort of dependency can quickly lead to maintenance problems, especially if
someone else is supposed to use the classes.
André Næss
André Næss
>> The information is typical formatting information. I think it would be
>> much cleaner if you allowed for two more arguments to getData() that
>> defined how many rows to retrieve, and what offset to start at. The
>> organization of data into pages belongs in presentation code.
>
> Rather have have extra arguments in my 'getData' method I have
> separate setters for $rows_per_page and $pageno, with a getter for
> $lastpage. The reason for this is that a default value for
> $rows_per_page can be defined in the class constructor.
But if you say that these variables are in fact selection criteria, then
certainly you must agree that seeing as $where is a collection of selection
criteria, and it's being passed to getData(), it would be much more
consistent to pass them as parameters!
>> From a purist perspective, SQL operates on sets, and so operations like
>> "get me item 1 through 10" makes no sense since sets have no intrinsic
>> ordering. But the second one applies the SORT operator, the result is no
>> longer a set, but a list -- a structure with a defined order, and hence
>> the retrieval of rows 1 through 10 suddenly makes sense.
>
> My point is that the presentation layer asks the database object to
> retrieve some data, and it is the database object is responsible for
> constructing the SQL statement. As the SQL statement contains the
> 'LIMIT x,y' clause it is necessary for the database object to have
> these values available. There is simply no other way.
And we agree on that. See my reply to "matty".
>> > 5) It is simple and it works, therefore it cannot be all that bad.
>>
>> I agree that it's simple and it does the job. But as far as I could
>> understand you wanted to submit your code to some sort of repository.
>> When code is to be used by many people it obviously has to be of much
>> higher quality than most of the code that's written every day, and --
>> more importantly -- it must adhere to the principles accepted by most of
>> the developers who are supposed to use the code.
>
> I have been a software developer for over 25 years and I have seen
> many different programmers with many different principles. Some try to
> produce code which is efficient, effective and maintainable then write
> a methodology to match, while others create a fancy methodology
> without any regard for efficiency or maintainability. As far as I am
> concerned the overriding principle is the KISS principle (Keep It
> Simple, Stupid). Too many people keep inventing stupid rules which
> just get in the way of efficient software development. Too many people
> have different ideas about what is *good* and *not good* practice. As
> far as I am concerned if it works, is simple and is maintainable then
> it is *good*. Anything else is *not good*.
KISS is a very good idea indeed. My point was that if you want to partake in
a community you have to follow the rules of the community, if you disagree,
you have to lobby your disagreement to the other members.
>> And in general I think your design is bad because you have to create a
>> new class for every table you create, and you have to recode both the
>> table and the class every time you change the table, which obviously
>> doubles the chances of bugs. I can't really see how this can save you
>> very much work I guess.
>
>
> You have missed the point about my abstract database class. Each
> database table has its own class which is a subclass of the abstract
> class, therefore it inherits all the properties and methods of the
> abstract class. Each table class therefore need only contain extra
> code which is specific to that class, such as a list of field names,
> validation rules, relationships, etc.
Precisely. And it is this code which leads to a problematic dependency
between the DB and the class. I understand the point of reusing code to
generate SELECT/INSERT/UPDATE/DELETE statements, but you can do that
without having to create a class for every table you have.
André Næss
matty
> Here's the relevant code:
> var $rows_per_page; // used in pagination
> var $pageno; // current page number
> var $lastpage; // highest page number
>
> So what Tony has done is put *pagination* information into the DB-class,
> and that is what I react to. He could insert two variables, $offset and
> $numRows, and that would be ok. But the pagination belongs in the
> presentation layer. It's a tiny difference, and not very important IMO,
> but I still think it's worth pointing out.
>
Well, certainly the values shouldn't be set in the constructor like
this, I agree. I still think that there is a place for a database class
that can handle pagination without outside help, although this does raise
issues as far as the heterogenous nature of the data it would have to
return (links, data values, etc)
> You have to write a new class which inherits the abstract class and has
> variables to represent the fields in the DB. Quote from Tony's page:
>
> function Database_Table ()
> {
> $this->tablename = 'default';
> $this->dbname = 'default';
> $this->rows_per_page = 10;
>
> $this->fieldlist = array('column1', 'column2', 'column3');
> $this->fieldlist['column1'] = array('pkey' => 'y');
> } // constructor
>
OK, I missed this one as I only scanned the article. Of course, a really
good DB class would identify the primary key itself, as the RDBMS can
provide this kind of information...
> So when you add a field to your DB, you also have to change the class,
> this sort of dependency can quickly lead to maintenance problems,
> especially if someone else is supposed to use the classes.
I agree entirely with you on this point - since the whole point of OO
is reusing code, not just an academic exercise, a class which cannot
be reused isn't very helpful!
Tony Marston
My method is correct according to the principles of the 3 tier
architecture in which the presentation layer talks to the business
layer which talks to the data access layer. It is not allowed for the
presentation layer to talk directly to the data access layer - it must
always go through thye business layer. Therefore (and this is the crux
of my argument) whatever information the data access layer needs to
construct the sql SELECT statement must go through the business layer.
That is why my business layer object contains variables for paging as
how else could the data access layer have the information required to
construct the LIMIT clause?
I think your idea of having data selection/limitation code in a
separate layer is totally wrong (but that's just my personal opinion).
> What if i wanted to use your db layer but didn't want the paging
> code? Is the class designed in such a way that i am forced to use it?
> Just things to consider.
If you want paging turned off (which I do in one of my test scripts)
then simply set $rows_per_page to zero, then my code will not generate
any LIMIT clause on the SELECT statement.
Tony Marston
http://www.tonymarston.net/
Tony Marston
> matty:
>
> > André Næss wrote:
<snip>
> Ok let me try to clarify this. To fetch rows 1 through 10 is ok. Using limit
> is ok. It's ok to define what subset of a sorted result set you want,
> because that's part of the selection. I agree.
>
> Here's the relevant code:
> var $rows_per_page; // used in pagination
> var $pageno; // current page number
> var $lastpage; // highest page number
>
> So what Tony has done is put *pagination* information into the DB-class, and
> that is what I react to. He could insert two variables, $offset and
> $numRows, and that would be ok. But the pagination belongs in the
> presentation layer. It's a tiny difference, and not very important IMO, but
> I still think it's worth pointing out.
You method of pagination is more complicated than mine, therefore
violates the KISS principle. In my method all the presentation object
has to do before calling the getData() method is as follows:
$dbobject->setRowsPerPage()
$dbobject->setPageNo()
Both of these are optional because there is a default value for
$rows_per_page inside the class, and $pageno will default to whatever
was used previously for this object in the current session.
My presentation layer does not calculate the offset as that is done
inside the data access layer - all it does is specify a page number
(which comes from a hyperlink in my pagination area).
<snip>
> You have to write a new class which inherits the abstract class and has
> variables to represent the fields in the DB. Quote from Tony's page:
>
> function Database_Table ()
> {
> $this->tablename = 'default';
> $this->dbname = 'default';
> $this->rows_per_page = 10;
>
> $this->fieldlist = array('column1', 'column2', 'column3');
> $this->fieldlist['column1'] = array('pkey' => 'y');
> } // constructor
>
> The variable $fieldlist is used to list all the columns within that table,
> and to identify which is the primary key. How this is used will become
> apparent
>
> End quote.
>
> So when you add a field to your DB, you also have to change the class, this
> sort of dependency can quickly lead to maintenance problems, especially if
> someone else is supposed to use the classes.
>
> André Næss
Having a separate class for each individual database table is supposed
to be a GOOD idea in OOP as it encapsulates all the information
required to read/write/update/delete that particular database table.
This includes a list of all fields and their characteristics so that
basic validation can be done by the code which is inherited from the
abstract class instead of having to write a separate set of validation
code for each table.
If the physical database table is changed then the class which
contains information about that table is also changed. This is not a
maintenance problem, it is basic common sense. If a programmer changes
a database table without updating the code which accesses that table
then he is a ****KING PLONKER of the first order. In my method there
is one class per table, so the changes have to be made in only one
place. What could be easier than that?
Tony Marston
http://tonymarston.net/
Tony Marston
> Hi Tony!
<snip>
> The others wrote everything I could write.
>
> My database class also supports a limit statement, I especially wrote
> it, because its such a hassle to limit with MSSQL.
>
> IMO, doesn't matter, if irt is parameter or class variable. I prefer
> parameter, because I don't like:
>
> $arith->x = 1;
> $arith->y = 2;
> $m = arith->multiply();
>
> I rather like:
>
> $m = arith->multiply(1,2);
>
> HTH, Jochen
>
The reason that I use separate variables instead of parameters on the
getData() method is that the number of possible parameters is quite
large and most of them are optional. So instead of having a long list
of parameters on the getData() method I find it easier to use a
separate set...() method whenever I DO have a value that I want to be
used. That is my personal preference. If I have a method which has a
small number of parameters then I would stick to supplying them on the
method call as you suggest.
The difference lies in the number of parameters and how many of them
are optional.
Tony Marston
http://www.tonymarston.net/
Tony Marston
> A part of your solution is Object Oriented but I have some considerations:
> Tables are part of a database, your approach only leaves room to work
> with one database.
WRONG! My current development uses 3 different databases within the
same application. Each class is named after the table, but the
database name is held within the class and is transparent to the
presentation layer.
> If you create a database class (as in: $db = new
> dataBase('server', 'username', 'password') you could have your table
> class make use of this dataBase class as in $table = new
> table($db->getLink()).
> Then you can distanciate yourself from the whereabouts of the tables and
> only worry about their data.
> Considering your getData: this requires you to have insight into the
> inner mechanics of your class (you have to set your desired pagesize,
> and you have to go through trouble if you don't want to get the standard
> subsequent pages, set by pagesize).
There is a default page size defined within each class, but it can be
changed at any time using the $dbobject->setRowsPerPage() method. A
zero value will turn off paging altogether.
> I suggest you have a method $table->getData(selection_criteria), which
> gives you every result there is. Then you could have a method
> $table->getPage(selection_criteria, pagesize='defaultvalue',
> pagenumber='1'). (the "=" will use a defaultvalue, if you don't pass
> that parameter).
> Now you can make a getNextPage(), getPrevPage(), ... which do what you
> can guess... :).
I don't want a separate method for firstpage(), previouspage(),
nextpage(), lastpage() when I can use a setPageNo() method followed by
my getData() method.
> I don't know anything about error-handling with php, but you might want
> to consider tying mysql-specific errors to your mysql-classes.
> $results = $table->getPage(...);
> if ($table->hasError())
> $table->printError();
> That way, with inheritance (and overriding) your $db errors can be
> delegated to $table...
Take a look at http://www.tonymarston.net/php-mysql/errorhandler.html
> I hope this has been of any help.
>
> ps: This is not written out of arrogance and I don't think I know it
> better than anyone else. I'm just a student, and also was pondering
> about a nice oo-approach to php/mysql scripting.
> This is just a mere braindump.
>
> Glen
You have to learn somehow, but something you have to watch out for is
that different people have different ideas about what is *good* or
*not good* programming. My advice is as follows:
- Try to follow the KISS principle and you won't go far wrong.
- When developing software there should only be 3 limiting factors:
1) The limits imposed by the user requirements.
2) The limits imposed by the chosen development language.
3) The limits of you own abilities.
Other limitations, such as people telling you which methodology is
*right* or even which interpretation of the methodology is *right*,
are entirely artificial and therefore can be ignored.
Tony Marston
http://www.tonymarston.net/
matty
<snip>
I have to disagree with you here! Having a separate class *instance* is
good OO, having a separate *class* is bad OO, since you lose all the
potential benefits of low maintenance, etc.
>
> If the physical database table is changed then the class which
> contains information about that table is also changed. This is not a
> maintenance problem, it is basic common sense. If a programmer changes
> a database table without updating the code which accesses that table
> then he is a ****KING PLONKER of the first order. In my method there
> is one class per table, so the changes have to be made in only one
> place. What could be easier than that?
>
How about something like this:
(obviously not a specific implementation, more a hypothetical description
of an interface)
$tablea = new DBTable($database, $user, $password, $table);
foreach(array('widgetid', 'manufr', 'cost', 'colour', 'weight') as $item)
{
$tablea->AddField($item);
}
$tablea->AddConstraint('cost > 20.00');
$tablea->AddConstraint('colour = \'blue\'');
if (!array_key_exists('offset', $_GET) or !preg_match('/^[0-9]+/', $_GET['offset']))
{
$dboffset = 0;
}
$resultarray = $tablea->GetPageList($offset, $config['database']['pagelength']);
Then you can change database, change fields, paging length, WHERE constraints, etc
without having to change the *class*, merely the class instance. You only have to change
DBTable class code if you're changing its functionality.
Otherwise, in a system with 12 database tables, having a different class for each
table? That's not good.
Matt
R. Rajesh Jeba Anbiah
Kudos! Nice work... My points regarding the OOP style:
1. I could see the way you pass values to the methods contradicts the
OOP style. For example:
global $dbconnect, $query; <=======//
$dbconnect = db_connect($this->dbname) or trigger_error("SQL",
E_USER_ERROR);
at the "updateRecord ($fieldarray)" method.
You could have used setDBLink($db_link) to the class....
2.
function Database_Table ()
{
$this->tablename = 'default';
$this->dbname = 'default';
$this->rows_per_page = 10;
$this->fieldlist = array('column1', 'column2', 'column3');
$this->fieldlist['column1'] = array('pkey' => 'y');
} // constructor
If you this style, there won't be any reusability. In the
languages that support associative arrays, you may use like...
function Database_Table ()
{
$this->table_settings = array(); //hash array
} // constructor
Then...
function setTableSettings($key, $value)
{
$this->table_settings[$key] = $value;
}
function getTableSettings($key)
{
return( $this->table_settings[$key] );
}
This is my point. I may be wrong though...
Anyway... keep writing... nice article...
---
"If there is a God, he must be a sadist!"
Email: rrjanbiah-at-Y!com
André Næss
> André Næss <andrena.spa...@ifi.uio.no> wrote in message
> news:<bh0iss$ba8$1...@maud.ifi.uio.no>...
>> matty:
>>
>> > André Næss wrote:
>
> <snip>
>
>> Ok let me try to clarify this. To fetch rows 1 through 10 is ok. Using
>> limit is ok. It's ok to define what subset of a sorted result set you
>> want, because that's part of the selection. I agree.
>>
>> Here's the relevant code:
>> var $rows_per_page; // used in pagination
>> var $pageno; // current page number
>> var $lastpage; // highest page number
>>
>> So what Tony has done is put *pagination* information into the DB-class,
>> and that is what I react to. He could insert two variables, $offset and
>> $numRows, and that would be ok. But the pagination belongs in the
>> presentation layer. It's a tiny difference, and not very important IMO,
>> but I still think it's worth pointing out.
>
> You method of pagination is more complicated than mine, therefore
> violates the KISS principle. In my method all the presentation object
> has to do before calling the getData() method is as follows:
How is it more complicated? All I suggest is that you let the presentation
layer handle a presentation issue. It's really simple to supply the
presentation layer with default values for the pagination, and if you want
to change those values you have to change them *somewhere*. It just makes
more sense to change them in the presentation layer and let the
presentation layer handle the translation from offset/page number to the
limit clause. Transparently, of course.
Now, if I was to code this I'd probably write a sort of
PagedPresentation thingy, which is meant for this sort of
situation. PagedPresentation should just supply a framework for
creating paged presentations, and it has to be coupled with a data
source. The data source supplies the PagedPresentation with data, and
the PagedPresentation transforms these data as defined by the
developer using some sort of template (a simple HTML/PHP mix is
sufficient).
But there obviously has to be a connection between the data source and
the presentation, because the presentation outputs stuff which in the
end has to result in $_GET variables which define what page to
display. And this data then has to be used to figure out what rows to
fetch.
So, we have to figure out how to implement this as clean as
possible. Let us start by considering the purist solution, which is to
fetch *all* the rows and supply them to the presentation layer, which
outputs the rows in question. How do we achieve this? Well, presumably
we have a structure where a presentation module must request data from
it's data source, so in the presentation module we have something like
this:
$collection = $source->getData();
The source defines *what* the data are, of course. In most cases a
data source is just an abstraction of a particular SQL query.
getData() returns an collection, because after all, we are requesting
a (possibly empty) collection of data.
So now the presentation layer is about to begin it's work, what we in
a PagedPresentation would expect is something like this:
$offset = $_GET['pageNum'] * $itemsPerPage;
$data = range($collection, $offset, $itemsPerPage)
applyTemplate($data);
So does this interface make sense? It does IMO, and it can be
implemented in a fashion that makes it as efficient as a solution
which supplies the offset and the number of items per page at an
earlier stage. Why? Because we don't have to actually perform the
query until the data are accessed, and they aren't really accessed
until applyTemplate() is called! In this case range() is a selection
utility which defines a certain subrange of the collection, but it doesn't
really have to do it, nor does $source need be an actual query result, they
are just a facade designed to make the interface coherent and highly
reusable, the implementation can be made as efficient as possible using any
trick in the book.
So let me stress that this was the *implementation* of PagedPresentation, to
use PagedPresentation we would expect an interface like this:
$source = new DataSource("SELECT ALL RED CARS FROM 1998");
$presentation = new PagedPresentation($source, $templateFile, $itemsPrPage);
$presentation->display();
All highly simplified, of course.
> $dbobject->setRowsPerPage()
> $dbobject->setPageNo()
>
> Both of these are optional because there is a default value for
> $rows_per_page inside the class, and $pageno will default to whatever
> was used previously for this object in the current session.
And the same can be done if you put this stuff in the presentation layer. I
don't see the problem.
> My presentation layer does not calculate the offset as that is done
> inside the data access layer - all it does is specify a page number
> (which comes from a hyperlink in my pagination area).
I you think that such a basic calculation violates KISS, then you're taking
it too far IMO. And besides, it's more important to apply KISS interfaces.
In this case you have a less than perfectly clean, and IMO less intuitive
interface.
>> You have to write a new class which inherits the abstract class and has
>> variables to represent the fields in the DB. Quote from Tony's page:
>>
>> function Database_Table ()
>> {
>> $this->tablename = 'default';
>> $this->dbname = 'default';
>> $this->rows_per_page = 10;
>>
>> $this->fieldlist = array('column1', 'column2', 'column3');
>> $this->fieldlist['column1'] = array('pkey' => 'y');
>> } // constructor
>>
>> The variable $fieldlist is used to list all the columns within that
>> table, and to identify which is the primary key. How this is used will
>> become apparent
>>
>> End quote.
>>
>> So when you add a field to your DB, you also have to change the class,
>> this sort of dependency can quickly lead to maintenance problems,
>> especially if someone else is supposed to use the classes.
>>
>
> Having a separate class for each individual database table is supposed
> to be a GOOD idea in OOP as i