refactoring and simplification suggesition [#613606]

Hello,

I'm working on creating an phone.eg.inc, and I thought testing it would be simply the matter of copying the file to the directory as a mini-plug-in, but I was wrong.

Looking at phone.module it seems that numerous, complex, and error prone modifications need to be made in order to install a new country sub-module.

However, much of the required code are simply repetitions of control blocks with literal constants that are themselves the country codes, from which the module names, when needed can be deducted.

For example function phone_field_info() has information about the module that should be retrieved from it each .inc file directly via calling function phone_uk_metadata(). Likewise, the format validation error messages.

I suggest a re-factoring of the module so that it looks in its directory for phone_xx.inc files and use the information contained within them directly. Is there a reason why this approach isn't favourable?

I hope I'm not repeating an issue that was previously mentioned.

Comment	File	Size	Author
#2	phone.module.diff.gz	2.93 KB	agharbeia
#1	phone.module.gz	6.25 KB	agharbeia

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

agharbeia CreditAttribution: agharbeia commented 24 October 2009 at 22:10

File	Size
phone.module.gz	6.25 KB

In the attached module, I tried to implement the change I suggested above.

The module now dynamically uses the .inc files that exist in its directory to present a list of available country formats, as well as provide the error messages, and the validation and formatting code.

A draw back which I'm aware of is the extra hit due to repetitive filesystem access.

I haven't made significant coding contributions to Drupal before and I'm not fluent in PHP, so I don't know whether it is possible to cache the module list information in a module-level variable and reuse it as needed instead of scanning the directory each time.

While testing, modules for cs and nz caused problems. I'm note sure whether it is my code or theirs, but disabling them resolved the errors so I'm assuming there's something wrong with them, probably deprecated coding style.