2
\$\begingroup\$

I'm relatively new to php and any advice is appreciated.

I retrieve some data from an external source and process them in a foreach loop to display the items. Now I'm not sure if the way I choose is the right way or if there is a better way for a situation like this.

And here the code I use:

define ("WSDL_SERVER",
"XXX");
$oSmartFeed= new SoapClient(WSDL_SERVER);
$oSessionHash= $oSmartFeed->login('XXX', 'XXX');
if(!$oSessionHash->HasError){
$sSessionHash= $oSessionHash->Records['sessionHash'];
$aConfig= array('limit'=>5, 'offset'=>2, 'feeds'=>[XXX]);
$aResult= $oSmartFeed-> getProductData ($sSessionHash, $aConfig);

echo '<ul>';

foreach ($aResult->Records AS $aRecord) {

    $produktName = $aRecord['productname'];
    $imageUrl = $aRecord['imagebigurl'];
    $price = $aRecord['currentprice'] . $aRecord['currency'];
    $affLink = $aRecord['deeplinkurl'];
    $productDescription = $aRecord['productdescriptionlong'];
    $versandKosten = $aRecord['shipping'] . ' €';

    ?>
       <li>
           <a href="<?php echo $affLink;?>">
               <img src="<?php echo $imageUrl; ?>" alt="<?php echo $produktName; ?>">
               <h3>
                   <?php echo $produktName; ?>
               </h3>
               <p>
                   <?php echo $productDescription;?>
               </p>
               <aside>
                   <span>
                       Price: <?php echo $price; ?>
                   </span>
                   <br>
                   <span>
                       Shipping: <?php echo $versandKosten; ?>
                   </span>
               </aside>
           </a>
       </li>
    <?php
}
echo '</ul>';
$oSmartFeed->logout($sSessionHash);
\$\endgroup\$
1
  • \$\begingroup\$ This code doesn't look like it will run as is. (Your if statement is missing its closing brace.) \$\endgroup\$
    – cHao
    May 9, 2017 at 20:55

1 Answer 1

1
\$\begingroup\$

Have you considered using a template engine vs. writing old-style PHP spaghetti code?


if(!$oSessionHash->HasError){

Reverse this conditional to get rid of the huge section of code within this conditional (whihc is actually not even closed with the code shown. So something like:

if($oSessionHash->HasError) {
    // die, exit, redirect or similar
}
// rest of code, now no longer in a conditional.

This strategy of existing early from scripts/function/methods will help you write much less fragile code.


A number of stylistic concerns:

  • Drop the whole $o*, $a*, $s* variable naming (which looks like you are trying to infer variable type in name). Your variables should have meaningful and specific names which convey their meaning. Also, if you use a good IDE, you won't need this.
  • Your spacing around assgniment operators, flow control structures, etc. is wildly inconsisten, making your code hard to read. If you are strating in PHP, I would recommend getting to know the PSR-1 and PSR-2 coding standards as well as other PSR recommendations which are most widely accetped ways of writing code in the profession PHP development community. A good IDE will be able to help you enforce these styles.
  • Don't split your define across two lines unnecessarily like you are.

Why not perform logout immediately after your API call response is returned, rather than wait until after displaying results?


This code is very happy path oriented. What happens if API doesn't respond? What if it responds with an error code? What if it returns empty result set?

\$\endgroup\$
3
  • \$\begingroup\$ Some great advices. Thank you. But could you give me an example or a hint for "template engine vs. writing old-style PHP spaghetti code"? I really have no idea what a php template engine is. \$\endgroup\$
    – Steve
    May 9, 2017 at 21:28
  • \$\begingroup\$ @Steve templates are used to help separate your code logic from your display logic. Twig is probably the most popular for PHP these days - twig.sensiolabs.org \$\endgroup\$
    – Mike Brant
    May 10, 2017 at 16:08
  • \$\begingroup\$ Ah, thank you. I will take a look into it. \$\endgroup\$
    – Steve
    May 11, 2017 at 13:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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