CodeSOD: Strings go Asplodey

Anton has the joy of doing PHP work using one of the more popular e-commerce platforms.

Now, say what you will about PHP, its absolute mess of a root namespace has a function or class for just about everything. You want to split a string? We call that explode because that's way more radical than split. Want to join a string back together? We use implode, because that's the opposite of explode.

Or, if you forget how to use implode, you could always write it yourself, which is what Anton found in the codebase:

<?php $flag = false ?><?php echo $this->__('Your order number is ') ?><?php foreach ($_orderIds as $orderId=>$incrementId): ?> <?php if ($flag): ?> <?php echo ', ' ?> <?php endif; ?> <?php $flag = true ?> <a href="<?php echo $this->getViewOrderUrl($orderId) ?>"><?php echo $incrementId ?></a><?php endforeach; ?>

Now, as "we reimplemented a built-in function" standards, this isn't the worst instance of that logic. The basic "for loop, with a flag to help you remember if you should add a comma or not," isn't absurd.

No, the real ugly I see here is the (ab)use of PHP's interpretation. Every statement is wrapped in its own <?php ?> block, which is not necessary, and certainly makes the code less readable. The whole (dubious) advantage of PHP is that you can mix code with literal HTML output. There's no reason to <?php echo ', '?> when we could just output a , and a space directly in the page. Or conversely, we could put most of the logic inside of a single <?php ?> block and echo as needed to generate output, since we're doing echos for pretty much everything anyway.

All in all, it's just ugly code. Widely used ugly code. (Or at least was widely used when Anton submitted this, which was some time ago- we could hope the code has been improved since then)

[Advertisement] ProGet's got you covered with security and access controls on your NuGet feeds. Learn more.