Comments for "HTML_TagCloud"

» Submit Your Comment
Comments are only accepted during the "Proposal" phase. This proposal is currently in the "Finished" phase.
» Comments
  • 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>&nbsp;\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)