| Justin Patrin 2006-05-24, 7:01 pm |
| On 5/23/06, Jan Wagner <wagner@netsols.de> wrote:
> Hi everyone,
>
> as promised a long time ago, I finally managed to put the mentioned
> elements into PEAR packages. These and examples can be found here:
>
> http://www.netsols.de/pear/htmlarea
> http://www.netsols.de/pear/fckeditor
> http://www.netsols.de/pear/jscalendar
>
> As these packages are really simple, I don't know if they are worth to
> be separate packages. The QuickForm authors mentioned once, that they
> would prefer it that way, but I'll wait for others to speak up.
>
Comments on the code:
"$name".....why?
As Phillipe said, don't use sprintf, especially for simple string
replacement. printf is *slow*. It's also a pain to read.
http://pear.reversefold.com/strings/
As shown in the above link, this is also slow:
",{$button['context']}"
I would prefer:
','.$button['context']
especially when you're concatenating a single char. I also like
concatenation better than " because it is syntax highlighted correctly
in all editors is just much easier to eyeball.
There is some inconsistent quote usage, " where you could be using '.
It is generally better practice to build a string as a literal then
run htmlentities() on it before you put it into HTML. If you'd rather
hard-code your & (no problems with this) then make sure to run
htmlentities() on any variables as it's possible they could have
entities that need to be escaped (such as BasePath).
Between the elements you're inconsistently using member vars vs.
options. For example, $this->BasePath in fckeditor and
$this->_options['basePath'] in jscalendar. I would prefer using the
options array over member variables.
If you're going to propose these to PEAR you also should look at your
variable naming. Member variables should be named with studlyCaps, not
AllCaps.
There also seems to be some overlap in JS functions, such ass
array2js. Perhaps you could make a base class for these elements which
holds these functions?
--=20
Justin Patrin
|