2
\$\begingroup\$

I'm using this PHP to modify A elements in a DOM fragment (a vBulletin $post['message'] if that matters) so that links to external sites always open new tabs. This is actually the default in vBulletin but it's not always reliable. Anyway, the code works, but looking at the loop makes me think it could be done better. Any suggestions?

$as = $doc->getElementsByTagName('a');
for ( $i = $as->length; --$i >= 0; ) {
    $a = $as->item($i);
    $aParts = parse_url ( strtolower ( $a->getAttribute('href') ) );
    if ( $aParts['host'] !== $localDomain ) {
        $a->setAttribute ( 'title', @$a->getAttribute ('title') . " [external link to {$aParts['host']}]" );
        $a->setAttribute ( 'target', '_blank' );
    }   // if
} // for
\$\endgroup\$
  • \$\begingroup\$ interesting that you inverted the loop and counted down from NodeList::length - did you have a particular reason for doing it that way? \$\endgroup\$ – prodigitalson Jun 29 '12 at 22:24
  • \$\begingroup\$ @prodigitalson I adapted that from another area where I'd removed an element during the loop; if you count from the beginning, the index gets messed up when you remove one, but if you count from the end it works. No particular reason for doing it that way this time, just some consistency. \$\endgroup\$ – AndrewRich Jun 29 '12 at 22:31
  • 2
    \$\begingroup\$ You literally just treat it the same way as you would an array. Like you could do this. \$\endgroup\$ – DaveRandom Jun 29 '12 at 22:52
  • 2
    \$\begingroup\$ If you look at the docs you'll see that DOMNodeList implements the Traversable interface - which means it will behave just like an array when used with foreach \$\endgroup\$ – DaveRandom Jun 29 '12 at 22:55
  • 1
    \$\begingroup\$ @Utkanos Careful now... You're in danger of getting flogged for heresy or burned as a witch... \$\endgroup\$ – DaveRandom Jun 29 '12 at 22:58
2
\$\begingroup\$

To make it elegant clean code I would:

  • make proper use of blank lines;
  • abide by the coding standards defined by PHP-FIG;
  • use the DOMNodeList object as an array so I can use foreach();
  • be consistent with either single ' or double quotes ";
  • use a guard clause.

Refactored code:

$aEls = $doc->getElementsByTagName('a');

foreach ($aEls as $a) {
    $url = parse_url(strtolower($a->getAttribute('href')));

    if ($url['host'] === $localDomain) {
        continue;
    }

    $title = $a->getAttribute('title') . ' [external link to ' . $url['host'] . ']';

    $a->setAttribute('title', $title);
    $a->setAttribute('target', '_blank');
}

(not tested)

\$\endgroup\$
1
\$\begingroup\$

DOM is a tree structure that consists of well defined objects (nodes) therefore I wouldn't use for loop.
Moreover, I don't know how for would influence the code if you need to do more radical actions like delete an element. I know for sure that if you delete an element while you are in a foreach loop you get an error and your changes are not committed.

There are 2 other easy ways to find elements in the DOM without using for or foreach:
- use Xpath - I would say the common and easiest approach because it doesn't need difficult coding
- use a recursive function to go through DOM; below how I would code this using an abstract class and closures. Another option is just to use a standard function in the global context but closures give some flexibility.

abstract class Utils
{
    static function loop(&$elem,$func)
    {
        $closure = function(&$el,$func){
            if($el->nodeType == 1 && $el->nodeName == "a"){
                $func = $fun->bindTo($el);
                $func($el);
                }
            else {
                if($el instanceof DOMNode && $el->hasChildNodes()){
                    $current = $el->firstChild;
                    while($current instanceof DOMNode){
                        self::loop($current,$func);
                        $current = $current->nextSibling;
                        }
                    }
                };
            };
        return $closure($elem,$func);
     }
}

Utils::loop($documentRootNode,function(&$a){
//do what you have to do here
//your A is either $a parameter or $this
//binding is not necessarily needed but is an option
});
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.