Re: Re: [CALL FOR VOTES] File_IMC

Marshall Roch wrote:

Davey wrote:

acount: Davey Vote: 0 Review: code review and example review I have not voted +1 because there is one problem which you can easily fix. the build method should be like so IMNSHO:

[snip]

You should check if the file_exists() before the include, rather than class_exists after so as to avoid any PHP E_NOTICE errors as you are raising your own error. You may want to do a class_exists() AS WELL after the include just to be sure its the right file or what not.

Your concerns, as well as Greg's, have been fixed in 0.3[1]. Thanks for the comments/votes. :)

Well, my vote is now +1 :) I have one more minor concern, which is really just a preference, I personally make all my errors at least, slightly different so you can easily see which one was pulled up when testing. So for example use these:

        if (!file_exists($filename)) {
            return File_IMC::raiseError(
                'No builder driver _found_ for format: ' . $format,
                FILE_IMC_ERROR_INVALID_DRIVER);
        }

        include_once $filename;

        if (!class_exists($classname)) {
            return File_IMC::raiseError(
                'No builder driver _exists_ for format: ' . $format,
                FILE_IMC_ERROR_INVALID_DRIVER);
        }

Just note the difference is the _words_ :) - Davey