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/s..._portfolio.sql
- SQL dump
http://simeons.net/code/datamapper/mysql_to_xml_oo.phps
- Calling script
http://simeons.net/code/datamapper/h.../autoload.phps
- Auto load classes
http://simeons.net/code/datamapper/classes/DB.phps
- ADOdb connection
http://simeons.net/code/datamapper/c...ySQLToXML.phps
- MySQL extraction and parsing
- XML writing

Here are the outputted xml files:
http://simeons.net/code/datamapper/x...maps/album.xml
http://simeons.net/code/datamapper/x...maps/photo.xml
http://simeons.net/code/datamapper/x...ps/comment.xml

Any comments are appreciated.

Thanks,
Simeon

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/s..._portfolio.sql
> - SQL dump
> http://simeons.net/code/datamapper/mysql_to_xml_oo.phps
> - Calling script
> http://simeons.net/code/datamapper/h.../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/c...ySQLToXML.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/x...maps/album.xml
> http://simeons.net/code/datamapper/x...maps/photo.xml
> http://simeons.net/code/datamapper/x...ps/comment.xml
>
> Any comments are appreciated.
>
> Thanks,
> Simeon
>

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/s..._portfolio.sql
>>> - SQL dump
>>> http://simeons.net/code/datamapper/mysql_to_xml_oo.phps
>>> - Calling script
>>> http://simeons.net/code/datamapper/h.../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/c...ySQLToXML.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/x...maps/album.xml
>>> http://simeons.net/code/datamapper/x...maps/photo.xml
>>> http://simeons.net/code/datamapper/x...ps/comment.xml
>>>
>>> Any comments are appreciated.
>>>
>>> Thanks,
>>> Simeon
>>>
>>
>

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

On Nov 21, 2007 2:05 PM, Simeon F. Willbanks <simeon@simeons.net> 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

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!
.................................................. ..........

On Nov 22, 2007 11:52 AM, Robert Cummings <robert@interjinn.com> 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/Compute...esign_Patterns

~Philip

2007/11/21, Simeon F. Willbanks <simeon@simeons.net>:
>
> 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/s..._portfolio.sql
> - SQL dump
> http://simeons.net/code/datamapper/mysql_to_xml_oo.phps
> - Calling script
> http://simeons.net/code/datamapper/h.../autoload.phps
> - Auto load classes
> http://simeons.net/code/datamapper/classes/DB.phps
> - ADOdb connection
> http://simeons.net/code/datamapper/c...ySQLToXML.phps
> - MySQL extraction and parsing
> - XML writing
>
> Here are the outputted xml files:
> http://simeons.net/code/datamapper/x...maps/album.xml
> http://simeons.net/code/datamapper/x...maps/photo.xml
> http://simeons.net/code/datamapper/x...ps/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.