Re: [PHP] Code Critique Please :)
On Nov 22, 2007 11:52 AM, Robert Cummings [EMAIL PROTECTED] wrote: On Thu, 2007-11-22 at 12:46 -0500, Oscar Gosdinski wrote: There is something that i always wonder about Singleton pattern in PHP, do you really have a benefit using this pattern in PHP? The idea behind this pattern is that only one instance of the class is created, it works great in Java because all requests are processed inside a JVM and this instance created will really be the only one defined. Because in PHP every request has its own environment, you will have several instances of this class per request processed. Doesn't matter... you may have multiple requests for the object within the same HTTP request. Singleton pattern is very valid in PHP. Cheers, Rob I don't know if I should be embarrassed or not, but I'd not heard of the Singleton pattern before. So, I enlightened myself and learned something new (and it's only 9:00 am on a Monday morning!). =D Google it or click here - it's pretty easy reading... http://en.wikibooks.org/wiki/Computer_Science/Design_Patterns ~Philip
Re: [PHP] Code Critique Please :)
2007/11/21, Simeon F. Willbanks [EMAIL PROTECTED]:
Hello,
I am trying to increase my knowledge and understanding of OO and OO
Design Patterns. I'd like to request a critique of a program that
extracts MySQL table information and translates it into XML. In the
program, I tried to accomplish a few things:
1. Correct use of ADOdb
2. Closely match the PEAR coding standards
- I encode my scripts in UTF-8
- Not all indents are 4 spaces, but I have switched my IDE to
treat
tabs as four spaces
- My phpDoc comments could probably use some tweaking
3. Object Oriented principles
4. Strategy Design Pattern
- Interface used for column attribute parsing
My overall goal is to use these xml files as a starting point to write
a DAO with the Data Mapper Pattern.
Here are the related files to peruse:
http://simeons.net/code/datamapper/sql/photo_portfolio.sql
- SQL dump
http://simeons.net/code/datamapper/mysql_to_xml_oo.phps
- Calling script
http://simeons.net/code/datamapper/helpers/autoload.phps
- Auto load classes
http://simeons.net/code/datamapper/classes/DB.phps
- ADOdb connection
http://simeons.net/code/datamapper/classes/MySQLToXML.phps
- MySQL extraction and parsing
- XML writing
Here are the outputted xml files:
http://simeons.net/code/datamapper/xmldatamaps/album.xml
http://simeons.net/code/datamapper/xmldatamaps/photo.xml
http://simeons.net/code/datamapper/xmldatamaps/comment.xml
Any comments are appreciated.
Thanks,
Simeon
--
PHP General Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php
My 2 cents:
1) Do not use __autoload. Use SPL's spl_autoload_register().
2) Are you using one file per class? If not, you should.
3) Class names should be as self-explanatory as possible.
They should indicate what the class purpose is.
For example: MySQLToXML is too ambiguous as a class name. What does it do?
What aspect of MySQL translates to an XML?
MySQLToXML could be the name of the class package but not the name of the
class. I would have named it: MySQLDataToXMLExporter. Or preceding the name
with the package name: MySQLToXML_XMLExporter.
4) Class names shouldn't be verb conjugations. Use nouns instead.
e.g.: ParseDatabaseColumnAttribute is wrong, use something like
DatabaseColumnExporter or DatabaseColumnConverter or DatabaseColumnParser
(is not clear what its purpose is).
5) The use of __get with unrestricted access breaks encapsulation.
Your definition of __get in MySQLToXML provides access to any private var.
Did you actually intended that?
It doesn't look good anyway. Seems like a nasty workaround.
6) Function names should explain their purposes.
_setXmlDocuments doesn't set xml documents, it's actually building them.
Also, functions that start with get and set are usually expected to be just
getter and setters, which is usually associated with O(1) operations, or
O(logN)/O(N) in containers (meaning that it's not expected that a get or set
functions does more than just get or set a variable).
7) DON'T BE A SPORT COMMENTATOR!
Putting a comment on each line of code and just saying what it does actually
generates the opposite effect. It harms code readability and
maintainability. Also, it's a common sign of someone who was just forced to
put comments and didn't know how to do it.
You should use inline comments where relevant. If code is self-explanatory
(to a certain degree), inline comments are just a bother. You should use
inline comments whenever the action being done by the code cannot be grasped
just by looking at it, or whenever the code differs too much from usual
constructions that someone might not see what it's actually doing.
For example, compare the following:
// set table name
$tableName = $tableResultSet-fields[0];
// set xml document version and character encoding
$this-_xmlDocuments[$tableName] = '?xml version=1.0 encoding=UTF-8?'
. \n;
// open _xmlDocuments element for this table
$this-_xmlDocuments[$tableName] .= '' . $tableName . '' . \n;
// fetch array with associative keys
$this-_dbConnection-setFetchMode(ADODB_FETCH_ASSOC);
// ADOdb result set of fields in the current table
$columnResultSet =
$this-_dbConnection-execute(sprintf($this-_dbConnection-metaColumnsSQL,
$tableName));
// loop until the end of the fields result set
while (!$columnResultSet-EOF) {
with this:
// loop initialization
$tableName = $tableResultSet-fields[0];
$this-_xmlDocuments[$tableName] = '?xml version=1.0 encoding=UTF-8?'
. \n;
$this-_xmlDocuments[$tableName] .= '' . $tableName . '' . \n;
// we need associative arrays
$this-_dbConnection-setFetchMode(ADODB_FETCH_ASSOC);
$columnResultSet =
$this-_dbConnection-execute(sprintf($this-_dbConnection-metaColumnsSQL,
$tableName));
// loop through every record
while (!$columnResultSet-EOF) {
8) It seems that it's not the strategy pattern what you're using but the
builder pattern.
I'm not the right
Re: [PHP] Code Critique Please :)
Simeon F. Willbanks wrote: Hello, I am trying to increase my knowledge and understanding of OO and OO Design Patterns. I'd like to request a critique of a program that extracts MySQL table information and translates it into XML. You know that mysql has an output option for producing XML ? /Per Jessen, Zürich -- PHP General Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP] Code Critique Please :)
No I did not, thanks! For those that need more information, here is a good tutorial: http://en.wikibooks.org/wiki/XML_-_Managing_Data_Exchange/Converting_MySQL_to_XML Simeon On Nov 22, 2007, at 10:05 AM, [EMAIL PROTECTED] wrote: You know that mysql has an output option for producing XML ? /Per Jessen, Zürich -- PHP General Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP] Code Critique Please :)
On Nov 21, 2007 2:05 PM, Simeon F. Willbanks [EMAIL PROTECTED] wrote: 3. Object Oriented principles I see that you tried to implement Singleton pattern in the DB class, but you have a mistake. $dbConnection attribute is not a static member, so every time you call the constructor $dbConnection won't be initialized, so your code will always initialize this attribute. There is something that i always wonder about Singleton pattern in PHP, do you really have a benefit using this pattern in PHP? The idea behind this pattern is that only one instance of the class is created, it works great in Java because all requests are processed inside a JVM and this instance created will really be the only one defined. Because in PHP every request has its own environment, you will have several instances of this class per request processed. 4. Strategy Design Pattern - Interface used for column attribute parsing I've checked the code in MySQLToXML.phps and i see a lot of ParseDatabaseColumnAttributeXXX classes that implements ParseDatabaseColumnAttribute interface. I think that those classes should be methods of a DatabaseColumnAttributeParser instead of defining so many classes. Also the names of those classes suggest me that they are methods not objects. -- Saludos Oscar -- PHP General Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP] Code Critique Please :)
On Thu, 2007-11-22 at 12:46 -0500, Oscar Gosdinski wrote: There is something that i always wonder about Singleton pattern in PHP, do you really have a benefit using this pattern in PHP? The idea behind this pattern is that only one instance of the class is created, it works great in Java because all requests are processed inside a JVM and this instance created will really be the only one defined. Because in PHP every request has its own environment, you will have several instances of this class per request processed. Doesn't matter... you may have multiple requests for the object within the same HTTP request. Singleton pattern is very valid in PHP. Cheers, Rob. -- ... SwarmBuy.com - http://www.swarmbuy.com Leveraging the buying power of the masses! ... -- PHP General Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
[PHP] Code Critique Please :)
Hello, I am trying to increase my knowledge and understanding of OO and OO Design Patterns. I'd like to request a critique of a program that extracts MySQL table information and translates it into XML. In the program, I tried to accomplish a few things: 1. Correct use of ADOdb 2. Closely match the PEAR coding standards - I encode my scripts in UTF-8 - Not all indents are 4 spaces, but I have switched my IDE to treat tabs as four spaces - My phpDoc comments could probably use some tweaking 3. Object Oriented principles 4. Strategy Design Pattern - Interface used for column attribute parsing My overall goal is to use these xml files as a starting point to write a DAO with the Data Mapper Pattern. Here are the related files to peruse: http://simeons.net/code/datamapper/sql/photo_portfolio.sql - SQL dump http://simeons.net/code/datamapper/mysql_to_xml_oo.phps - Calling script http://simeons.net/code/datamapper/helpers/autoload.phps - Auto load classes http://simeons.net/code/datamapper/classes/DB.phps - ADOdb connection http://simeons.net/code/datamapper/classes/MySQLToXML.phps - MySQL extraction and parsing - XML writing Here are the outputted xml files: http://simeons.net/code/datamapper/xmldatamaps/album.xml http://simeons.net/code/datamapper/xmldatamaps/photo.xml http://simeons.net/code/datamapper/xmldatamaps/comment.xml Any comments are appreciated. Thanks, Simeon -- PHP General Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP] Code Critique Please :)
Simeon F. Willbanks wrote: Hello, I am trying to increase my knowledge and understanding of OO and OO Design Patterns. I'd like to request a critique of a program that extracts MySQL table information and translates it into XML. In the program, I tried to accomplish a few things: 1. Correct use of ADOdb 2. Closely match the PEAR coding standards - I encode my scripts in UTF-8 - Not all indents are 4 spaces, but I have switched my IDE to treat tabs as four spaces - My phpDoc comments could probably use some tweaking 3. Object Oriented principles 4. Strategy Design Pattern - Interface used for column attribute parsing My overall goal is to use these xml files as a starting point to write a DAO with the Data Mapper Pattern. Here are the related files to peruse: http://simeons.net/code/datamapper/sql/photo_portfolio.sql - SQL dump http://simeons.net/code/datamapper/mysql_to_xml_oo.phps - Calling script http://simeons.net/code/datamapper/helpers/autoload.phps - Auto load classes does the preg_replace() need to be run for every call to __autoload(), I think it would speed it up a little if you stored the class path in a static variable so that you only have to determine it once. http://simeons.net/code/datamapper/classes/DB.phps - ADOdb connection http://simeons.net/code/datamapper/classes/MySQLToXML.phps - MySQL extraction and parsing - XML writing I'd drop the trailing '?' in all your files - it's not required and it will save you hunting through stacks of files trying to find errant blank lines (which are output to the browser and will break subsequent calls to header()) Here are the outputted xml files: http://simeons.net/code/datamapper/xmldatamaps/album.xml http://simeons.net/code/datamapper/xmldatamaps/photo.xml http://simeons.net/code/datamapper/xmldatamaps/comment.xml Any comments are appreciated. Thanks, Simeon -- PHP General Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP] Code Critique Please :)
Simeon F. Willbanks wrote: Jochem, Thanks for the tips. no worries, for the rest you stuff looks tidy and making use of phpDoc comments and using CS consistently are both recommended! with regard to OO I didn't really see enough [complexity] to be able to offer any worthwhile feedback - but then again I didn't see any faux pas' either. you might consider checking out other implementations of similar functionality to compare your own idea/implementation against. oh and we prefer to keep all communications on list :-) Simeon On Nov 21, 2007, at 2:23 PM, Jochem Maas wrote: Simeon F. Willbanks wrote: Hello, I am trying to increase my knowledge and understanding of OO and OO Design Patterns. I'd like to request a critique of a program that extracts MySQL table information and translates it into XML. In the program, I tried to accomplish a few things: 1. Correct use of ADOdb 2. Closely match the PEAR coding standards - I encode my scripts in UTF-8 - Not all indents are 4 spaces, but I have switched my IDE to treat tabs as four spaces - My phpDoc comments could probably use some tweaking 3. Object Oriented principles 4. Strategy Design Pattern - Interface used for column attribute parsing My overall goal is to use these xml files as a starting point to write a DAO with the Data Mapper Pattern. Here are the related files to peruse: http://simeons.net/code/datamapper/sql/photo_portfolio.sql - SQL dump http://simeons.net/code/datamapper/mysql_to_xml_oo.phps - Calling script http://simeons.net/code/datamapper/helpers/autoload.phps - Auto load classes does the preg_replace() need to be run for every call to __autoload(), I think it would speed it up a little if you stored the class path in a static variable so that you only have to determine it once. http://simeons.net/code/datamapper/classes/DB.phps - ADOdb connection http://simeons.net/code/datamapper/classes/MySQLToXML.phps - MySQL extraction and parsing - XML writing I'd drop the trailing '?' in all your files - it's not required and it will save you hunting through stacks of files trying to find errant blank lines (which are output to the browser and will break subsequent calls to header()) Here are the outputted xml files: http://simeons.net/code/datamapper/xmldatamaps/album.xml http://simeons.net/code/datamapper/xmldatamaps/photo.xml http://simeons.net/code/datamapper/xmldatamaps/comment.xml Any comments are appreciated. Thanks, Simeon -- PHP General Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
