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 person to explain the difference, but I'll try.
The strategy pattern is used to separate an algorithm from its implementor.
The builder pattern is used when you're building a multi-part structure and
you don't want to statically bind the building process to a particular
abstraction. This is what I think you're doing. You need to build the XML,
and delegate the construction to an abstraction who actually knows how to
build its parts, through a common interface. You could use a strategy
pattern if you need to output in different formats, each shares the same
data structure but the algorithm to do the export is different.
