Refactoring help

[terrid25]

Hi

 

I have the following code:

 

    $image_1 = $value->getElementsByTagName("Image1");
    $image1  = $image_1->item(0)->nodeValue;

    $image_2 = $value->getElementsByTagName("Image2");
    $image2  = $image_2->item(0)->nodeValue;

    $image_3 = $value->getElementsByTagName("Image3");
    $image3  = $image_3->item(0)->nodeValue;

    $filename_1 = basename($image1);
    $ch = curl_init ($image1);
    curl_setopt($ch, CURLOPT_HEADER, 0);
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
    curl_setopt($ch, CURLOPT_BINARYTRANSFER,1);
    $rawdata=curl_exec ($ch);
    curl_close ($ch);
    $fp = fopen(Mage::getBaseDir('media') . DS . 'import/'.$filename_1,'w');
    fwrite($fp, $rawdata);
    fclose($fp);

    $filename_2 = basename($image2);
    $ch = curl_init ($image2);
    curl_setopt($ch, CURLOPT_HEADER, 0);
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
    curl_setopt($ch, CURLOPT_BINARYTRANSFER,1);
    $rawdata=curl_exec ($ch);
    curl_close ($ch);
    $fp = fopen(Mage::getBaseDir('media') . DS . 'import/'.$filename_2,'w');
    fwrite($fp, $rawdata);
    fclose($fp);

    $filename_3 = basename($image3);
    $ch = curl_init ($image3);
    curl_setopt($ch, CURLOPT_HEADER, 0);
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
    curl_setopt($ch, CURLOPT_BINARYTRANSFER,1);
    $rawdata=curl_exec ($ch);
    curl_close ($ch);
    $fp = fopen(Mage::getBaseDir('media') . DS . 'import/'.$filename_3,'w');
    fwrite($fp, $rawdata);
    fclose($fp);

 

Would someone help me to refactor this, as I can seem needing $image_4, $image_5 at a later date.

 

Thank you

[kenrbnsn]

Whenever you see repeated code like that you should replace that code with a function:

<?php
function do_work($value, $num) {

    $image = $value->getElementsByTagName("Image$num");
    $imagex  = $image->item(0)->nodeValue;

    $filename = basename($imagex);
    $ch = curl_init ($imagex);
    curl_setopt($ch, CURLOPT_HEADER, 0);
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
    curl_setopt($ch, CURLOPT_BINARYTRANSFER,1);
    $rawdata=curl_exec ($ch);
    curl_close ($ch);
    $fp = fopen(Mage::getBaseDir('media') . DS . 'import/'.$filename,'w');
    fwrite($fp, $rawdata);
    fclose($fp);
}

for ($i=1;$i<4;++$i) {
    do_work($value,$i);
}
?>

 

Ken

[ignace]

Always think in reusable components, here's an example derived from your code:

 

function do($name) {
    $image3 = do1($value, $name);
    $filename_3 = basename($image3);
    $rawdata = do2($image3);
    do3(Mage::getBaseDir('media') . DS . 'import/'.$filename_3);
}

function do1($value, $name) {
    return $value->getElementsByTagName($name)->item(0)->nodeValue;
}

function do2($image3) {
    $ch = curl_init($image3);
    curl_setopt($ch, CURLOPT_HEADER, 0);
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
    curl_setopt($ch, CURLOPT_BINARYTRANSFER,1);
    $rawdata=curl_exec($ch);
    curl_close($ch);
    return $rawdata;
}

function do3($filename_3) {
    $fp = fopen($filename_3,'w');
    fwrite($fp, $rawdata);
    fclose($fp);
}

 

The above code won't run but the important thing about this code is that it shows the principles applied. If you have a big chunk of code, look for distinct tasks executed by your code and wrap each into a function.