Re: [PEPr] Comment on Images::Image_QRCode

Hi Brett, On Wed, Dec 16, 2009 at 8:59 PM, Brett Bieber <brett.bieber@gmail.com>wrote: > > A few suggestions: > * I would declare the private variables as such, and use protected if > possible. > * The constructor should be named __construct since you're requiring PHP > 5. > * The first release should not be 1.0.0, maybe 0.1.0? > * Have you tried installing the package and running it? The data files > will not be placed into the same directory as the php files unless their > role is php. Just be aware of this and either use an install-time > replacement or find some other way to reference the files. > * I'm not sure if some of the $options passed to makeCode could be made > into constructor config options or not, for example, you can only set the > image type in the makeCode() and not in the constructor which seems a bit > odd since it's a member var of the class. > > > Also note - I've no clue if the implementation works as I have no need for > something like this yet. :-) > > There are a few methods which are pretty large, but overall the code looks > pretty good with helpful inline comments and a lot of phpdoc declarations. > Thanks for the above comments, much appreciated. I'll make the changes you suggest, including the versioning change which does make more sense :-) Re. the $options and in particular the image type, the thinking was that you could generate multiple codes using a single instantiation of the class with a) different data and b) different image types. But maybe it does make more sense to shift the options into the constructor configuration options, and keep makeCode's parameters to just the data. Thoughts welcome :-) Cheers, Rich -- Rich Sage Oxford, UK rich.sage@gmail.com