Vote Details for "Address_Book" by alan_k

» Details
  • Voter: Alan Knowles 
  • Vote: +1 (conditional)
  • Reviews: Cursory source review
» Comment
* Category: I think "File Formats" is more suitable
* Name: Contact_AddressBook ? - more in line with exisiting packages.

* Code
- safemode checking for fopen seems counter intuative - just remove the @ from fopen, and expose the error if the system is set up wrong - let the sysadmin fix it..
- PHP_Compat dependancy would be better as optional (and only loaded if the version was less that 4.3)
- Address_Book_Parser::getFileContents($filename) ?? file_get_contents / or pull in PHP_Compat?
- Address_Book_Builder_csv_outlook_express
associative array 0 => '...', 1=>'...' is very odd (you dont need the keys?)
- The conversion logic is a little odd, rather than picking a neutral format (eg. find an rfc etc.) - it is very confusing using number maps in some places - key maps in others.
- There is no clear internal storage format - it looks a bit like there is a numeric array to store/retrieve the data make the code very difficult to follow.
- remove/reduce @ usage - especially infront of 'include'. (if this fails it will be near impossible to solve. and is alot easier to isolate the issue if there is an error message.)
- pass by ref: I'm not sure you really need to do this everywhere.