Olivier Guilyardi [2006-06-14 10:42 UTC] Very nice !
Remarks :
- the strtotime() calls in TagCloud_example1.php fail on my system (PHP 4.4.2), the "YYYYMMDDHHMMSS" format does not seem to be supported with my version of PHP (btw, please use quotes around strings...). "YYYY-MM-DD" works fine.
- your properties/methods naming scheme is not consistent. Example : $basefontsize, $_max_epoc, addElement(). Why dont you stick with the fooBar syntax ?
- I personally dislike public properties. One day or the other that ends up with breaking BC. IMO, options arrays (passed as method arguments) or setters/getters are better.
Olivier Guilyardi [2006-06-15 10:49 UTC] one more remark :
Why generating links as :
<span class="..." style="font-size: ..."><a href="...">...</a></span>
instead of :
<a class="..." style="font-size: ..." href="...">...</a>
?
Isn't span useless here ?
Olivier Guilyardi [2006-06-15 13:51 UTC] There's a small bug : the second argument of the constructor ($fontsizerange) has no effect.
You forgot to assign the corresponding property. Add :
$this->fontsizerange = $fontsizerange;
I also modified _createHTMLTag, to not use <span>, and to add a space at the end of line (that's for IE ; otherwise words are not spaced enough). That's the way they do at flickr.com :
function _createHTMLTag($tag, $type, $fontsize){
return sprintf("<a class=\"%s\" style=\"font-size: %d%s\" href=\"%s\">%s</a> \n",
You still need to change the CSS generation routines accordingly...
Justin Patrin [2006-06-15 15:03 UTC] Please be consistent with your quote usage. For example:
"<style type=".'"text/css">'."\n";
There is no reason for the initial " string.
'<style type="text/css">'."\n";
I would prefer ' for all strings as it is generally faster ("\n" and similar are excepted of course). See: http://pear.reversefold.com/strings/
See the above link also for the speed difference between ' and " when concatenating variables. ' is always faster.
Do not use sprintf for simple concatenation (%s). It is far slower than any other method and is simply not needed (printf should only be used for heavy formatting).
Looks good otherwise. :-)
Martin Jansen [2006-06-15 21:46 UTC] Any reason why this code still supports PHP 4? Everbody is in frenzy about E_STRICT compliance these days and yet we are still adding new packages that are far away from E_STRICT safety.
Shoma Suzuki [2006-06-16 03:02 UTC] Tanks for many comments.
I do the following by the end of next week.
- fix example1 for PHP4.
- fix CSS style in the way that suggested by Olivier.
- fix a bug of
- fix quote usage.
- Do not use sprintf().
- properties/methods naming scheme.
Mark Wiesemann [2006-07-17 19:53 UTC] What's the status here? I really would like to see this in PEAR -- although it is a small package, it is nice and useful.
Mark Wiesemann [2006-07-19 19:11 UTC] Can you please remove the file tag for the TGZ file from the package.xml file? I can't install the package because of this entry.
In the examples files, please use
require_once 'HTML/TagCloud.php';
instead of
require_once '../../TagCloud.php';
Not sure: do private properties still need to be prefixed with an underscore for PHP5-only code?
Personal preference: Names with camel caps are easier to read, e.g. $fontSizeRange instead of $fontsizerange.
CS (Coding Standards):
- try to make a linebreak after max. 80 characters
- the opening bracket ({} needs to be at the beginning of the next line
Nit-picky thing: s/PHP versions 5/PHP version 5/ in the header comment block.
About the license: Is there a reason for not using version 3.01 instead of 3.0 of the PHP License?
Mark Wiesemann [2006-07-19 19:14 UTC] > - the opening bracket ({} needs to be at the beginning of the next line
I meant the opening bracket of function definitions:
public function foo($param1, $param2, ...)
{
Mark Wiesemann [2006-07-25 09:35 UTC] Apart from minor spacing issues like
if($this->baseFontSize - $this->fontSizeRange > 0){
or
foreach($this->epocLevel as $item){
this looks good now, nice work.
BTW: If you use these funny function separators (don't know how they are really called), you should keep them in sync with the real prototypes, e.g.:
// {{{ private _createHTMLTag($tagname, $epocLevel, $fontSize)
protected function _createHTMLTag($tag, $type, $fontSize)
|
