2
\$\begingroup\$

Basically I am in argument with my colleague, I say this template (view) has to be fully refactored and all instances of duplicate code has to be removed. He disagrees and don't want me to touch it. Can you please do a code review as 3rd party opinion? This is magento phtml template file for checkout review step.

<?php echo $this->getMessagesBlock()->getGroupedHtml() ?>
<span class="gift-message-hover-text">
<?php echo $this->getCmsBlock( 'securecheckout_gift_message_hover_text' ); ?>
</span>
<div style="clear:both;"></div>
<form action="<?php echo $this->getUrl( 'securecheckout/user/submitreview' ); ?>" method="POST" id="checkout_review">
<?php
    //Init.
    $oCustomerSession    = $this->getCustomerSession();

    // @TODO cleanup and finalize.
    $aShippingData       = $oCustomerSession[ 'shipping' ];
    $oQuote              = $this->getQuote();
    $sCheckoutButtonHtml = '';

    // Real quote stored data ready to become an order
    $oShipping       = $oQuote->getShippingAddress();
    $oBilling        = $oQuote->getBillingAddress();
    $oShippingMethod = $oShipping->getShippingMethod();
    $oPayment        = $oQuote->getPayment();
    // v( $oPayment );
    $sPromo           = $this->helper( 'checkout_securecheckout/promo' )->getPromo();
    $sGiftCard        = $this->helper( 'checkout_securecheckout/giftcard' )->getGiftCard();

    // @IMPORTANT quote may not have all the address info.
    // Either ensure quote is properly set or use customer address ids.
    $iShippingAddressId = $oShipping->getData( 'customer_address_id' );
    $iBillingAddressId  = $oBilling->getData( 'customer_address_id' );
    $oCustomerShipping = Mage::getModel( 'customer/address' )->load( $iShippingAddressId );
    $oCustomerBilling = Mage::getModel( 'customer/address' )->load( $iBillingAddressId );

// v( $oShipping );
// v( $oBilling );
// v( $oCustomerShipping );
// v( $oCustomerBilling );
// v( $oShippingMethod );
// v( $oPayment );


    // @TODO ensure checkout is allowed only once all of our data checks complete.
    // @TODO handle virtual items/tbooks/gift cards (not yet but soon), donations?
    if( !empty( $oShipping ) && !empty( $oBilling ) && !empty( $oShippingMethod ) && !empty( $oPayment ) )
    {
        $sCheckoutButtonHtml .= '';
        $sCheckoutButtonHtml .= '<input type="checkbox" name="review[agree]" id="agree" required /><label for="agree" class="unselectable"> I Agree to the terms</label> ';
        $sCheckoutButtonHtml .= '<span>';
        $sCheckoutButtonHtml .= '<a href="//www.checkoutimes.com/content/help/rights/terms/terms-of-service.html" target="_blank">Terms of Service</a> ';
        $sCheckoutButtonHtml .= '<a href="//www.checkoutimes.com/content/help/rights/privacy/policy/privacy-policy.html" target="_blank">Privacy Policy</a> ';
        $sCheckoutButtonHtml .= '<a href="';
        $sCheckoutButtonHtml .= $this->getUrl( 'store-policies' );
        $sCheckoutButtonHtml .= '" target="_blank">Store Policies</a>';
        $sCheckoutButtonHtml .= '<br />';
        $sCheckoutButtonHtml .= '<button type="submit" name="continue" value="1">PLACE ORDER</button>';
        $sCheckoutButtonHtml .= '</span>';
    }
    $sPromo = $this->helper( 'checkout_securecheckout/promo' )->getPromo();
    $sEmail         = ( ( $oShipping->getData( 'email' ) ) ? $oShipping->getData( 'email' ) : '' );
    $sGiftCard        = $this->helper( 'checkout_securecheckout/giftcard' )->getGiftCard();
?>
<div style="clear:both;"></div>

<div class="review-cart">
<?php
    $oQuote        = $this->getQuote();
    $oCartItems    = $oQuote->getAllItems();
    $oOptionHelper = Mage::helper('catalog/product_configuration');
    // @TODO avoid html tables when outputting product info.
    foreach( $oCartItems as $oItem ):
        $oProduct = $oItem->getProduct();
?>
    <span>
        <a class="product-image" href="<?php echo $oProduct->getProductUrl(); ?>" title="<?php echo $this->escapeHtml($oProduct->getName()); ?>">
            <img src="<?php echo $this->helper('catalog/image')->init($oProduct, 'small_image')->resize(113, 113); ?>" width="113" height="113" alt="<?php echo $this->escapeHtml($oProduct->getName()); ?>" />
        </a>
    </span>
    <span>
        <h5 class="product-name"><a href="<?php echo $oProduct->getProductUrl(); ?>" title="<?php echo $this->escapeHtml($oProduct->getName()); ?>"><?php echo $this->escapeHtml($oProduct->getName()); ?></a></h5>
    </span>
    <span>
        <?php echo $oItem->getData( 'qty' ) . ' x '; ?>
        <?php echo '$' . sprintf( '%.2f', $oItem->getData( 'price' ) ); ?>
    </span>
    <span>
        <div class="item-options">
        <?php
        $aOptions = $oOptionHelper->getCustomOptions( $oItem );
        if( $aOptions ):
        ?>
            <?php foreach( $aOptions as $aOption ): ?>
            <?php $_formatedOptionValue = $this->getFormatedOptionValue( $aOption ) ?>
            <div class="options">
                <strong><?php echo $this->escapeHtml( $aOption['label'] ); ?> :</strong>
                <?php echo $aOption[ 'value' ] ?>
            </div>
            <?php endforeach; ?>
        <?php endif; ?>
        </div>
    </span>
<?php endforeach; ?>
</div>
<hr />

<?php // Order Shipping Address Section ?>
<div class="review-address">
<h5>Shipping Address</h5>
<a href="<?php echo $this->getUrl( 'securecheckout/guest/address' ); ?>">Edit</a>
<ul>
    <li>
        <?php echo ( empty( $oCustomerShipping[ 'firstname' ] ) ? '' : $oCustomerShipping[ 'firstname' ] ); ?>
        <?php echo ' '; ?>
        <?php echo ( empty( $oCustomerShipping[ 'lastname' ] ) ? '' : $oCustomerShipping[ 'lastname' ] ); ?>
    </li>
    <li>
        <?php echo ( empty( $oCustomerShipping[ 'company' ] ) ? '' : $oCustomerShipping[ 'company' ] ); ?>
    </li>
    <li>
        <?php echo ( ( $oCustomerShipping->getStreet( 1 ) )    ? $oCustomerShipping->getStreet( 1 ) : '' ); ?>
    </li>
    <li>
        <?php echo ( ( $oCustomerShipping->getStreet( 2 ) )    ? $oCustomerShipping->getStreet( 2 ) : '' ); ?>
    </li>
    <li>
        <?php echo ( empty( $oCustomerShipping[ 'city' ] ) ? '' : $oCustomerShipping[ 'city' ] ) . ', '; ?>
        <?php echo ( empty( $oCustomerShipping[ 'region' ] ) ? '' : $oCustomerShipping[ 'region' ] ) . ' '; ?>
        <?php echo ( empty( $oCustomerShipping[ 'postcode' ] ) ? '' : $oCustomerShipping[ 'postcode' ] ); ?>
    </li>
    <li>
            <?php echo ( empty( $oCustomerShipping[ 'telephone' ] ) ? '' : $oCustomerShipping[ 'telephone' ] ); ?>
    </li>
    <li>
        <?php // @TODO handle question mark hover on desktop as a helper, on mobile make it clickable ?>
        <span>Gift Message (Optional)</span><a href="#" title="Give your gift a personal touch. Include a special message for your recipient on the order packing slip at no extra charge! (Packing slip does not show pricing information.)" class="tooltip checkout">?</a>
    </li>
    <li>
        <textarea name="review[gift_message]" id="gift_message"><?php echo $this->helper( 'checkout_securecheckout/giftmessage' )->getGiftMessage(); ?></textarea>
    </li>
</ul>
</div>
<hr />

<div class="review-shipping">
<h5>Shipping Method</h5>
<br />
<?php echo $oShipping->getShippingMethod(); ?>
<a href="<?php echo $this->getUrl( 'securecheckout/guest/shipping' ); ?>"> Edit</a>
<ul>
    <li>
        <ul>
            <li>Have Promo Code</li>
            <?php echo $sPromo; ?>
        </ul>
    </li>
    <li>
        <ul>
            <li>Order Contact</li>
            <?php echo $sEmail; ?>
            <li><a href="<?php echo $this->getUrl( 'securecheckout/guest/shipping' ); ?>">Edit</a></li>
        </ul>
    </li>
</ul>
</div>
<hr />

<div class="review-payment">
<h5>Payment Method</h5>
<ul>
    <li>
        <?php if( $oPayment[ 'method' ] === 'purchaseorder' ): ?>
            PO#: <?php echo ( empty( $oPayment[ 'po_number' ] ) ? '' : $oPayment[ 'po_number' ] ); ?>
        <?php elseif( $oPayment[ 'method' ] === 'paypal_express' ): ?>
            <img src="<?php echo Mage::getDesign()->getSkinUrl( 'images/paypal-review-logo.png' ); ?>" alt="PayPal Logo" />
            <br />
            <?php
            echo ( ( isset( $oPayment[ 'additional_information' ][ 'paypal_payer_email' ] ) ) ? $oPayment[ 'additional_information' ][ 'paypal_payer_email' ] : '' );
            ?>
        <?php elseif( $oPayment[ 'method' ] === 'paymentech' ): ?>
            <?php echo $oPayment[ 'cc_type' ]; ?> ending in <?php echo $oPayment[ 'cc_last4' ]; ?>
        <?php endif; ?>
    </li>
    <li>
        Billing Address
    </li>
    <li>
        <?php echo ( empty( $oCustomerBilling[ 'firstname' ] ) ? '' : $oCustomerBilling[ 'firstname' ] ); ?>
        <?php echo ' '; ?>
        <?php echo ( empty( $oCustomerBilling[ 'lastname' ] ) ? '' : $oCustomerBilling[ 'lastname' ] ); ?>
    </li>
    <li>
        <?php echo ( empty( $oCustomerBilling[ 'company' ] ) ? '' : $oCustomerBilling[ 'company' ] ); ?>
    </li>
    <li>
        <?php echo ( ( $oCustomerBilling->getStreet( 1 ) )    ? $oCustomerBilling->getStreet( 1 ) : '' ); ?>
    </li>
    <li>
        <?php echo ( ( $oCustomerBilling->getStreet( 2 ) )    ? $oCustomerBilling->getStreet( 2 ) : '' ); ?>
    </li>
    <li>
        <?php echo ( empty( $oCustomerBilling[ 'city' ] ) ? '' : $oCustomerBilling[ 'city' ] ) . ', '; ?>
        <?php echo ( empty( $oCustomerBilling[ 'region' ] ) ? '' : $oCustomerBilling[ 'region' ] ) . ' '; ?>
        <?php echo ( empty( $oCustomerBilling[ 'postcode' ] ) ? '' : $oCustomerBilling[ 'postcode' ] ); ?>
    </li>
    <li>
        <?php echo ( empty( $oCustomerBilling[ 'telephone' ] ) ? '' : $oCustomerBilling[ 'telephone' ] ); ?>
    </li>
    <?php if( $sGiftCard ): ?>
    <li>
        Gift Card Information
    </li>
    <li>
        <?php echo $sGiftCard; ?>
    </li>
    <?php endif; ?>
</ul>
</div>
<hr />

<?php echo $sCheckoutButtonHtml; ?>
</form>
\$\endgroup\$
  • \$\begingroup\$ You can always point your colleague towards the '"boyscount rule": Always leave the campground (code) cleaner than you found it. A simple "cleanup" would be using <?= instead of <?php echo \$\endgroup\$ – Ronald Swets Mar 15 '16 at 9:16
1
\$\begingroup\$

I'm sure your colleague has their reasons for wanting to keep the code as it is, but lets put it like this: If I were to write this code from scratch, I wouldn't write it like this.

First of, the code has obvious duplication (eg billing and shipping addresses), which really hurt readability.

The main issue for me would be that it is a lot of code that does widely different things: it handles addresses, payments, product info, and so on. If you want to change something, you have to look through all of that just to find what you want. If it is handled everywhere like this, that will take quite some time.

The benefit of templates is that you can separate these different things into different files, and reuse them. For example, I'm sure that you need to display the billing address in the backend as well. Or in the user profile. Or ... Same is likely true for a short product overview, etc. Because of this, I'm guessing that you also duplicate code across files, which isn't good. It makes code less readable and a nightmare to maintain (you have to change it in every file, or everything looks - and works - slightly different everywhere).

I'm not that familiar with magento templates, but just one thing: I'm pretty sure that magento doesn't have automatic XSS protection in their templates, so something like echo ( empty( $oCustomerShipping[ 'firstname' ] ) ? '' : $oCustomerShipping[ 'firstname' ] ); looks open to XSS (that's just one example, it happens all over the place; sometimes you do use escapeHtml, but not nearly everywhere).

\$\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.