PHP - simple refactor

Question

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;

Is there an easier way, to not repeat code If I need $image_3 ?

i.e. how can I refactor this?

Thanks

UPDATE:

I am using the $images_x variables in further code, that also needs refactoring:

UPDATE 2: - My full 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_1=curl_exec ($ch);
curl_close ($ch);
$fp = fopen(Mage::getBaseDir('media') . DS . 'import/'.$filename_1,'w');
fwrite($fp, $rawdata_1);
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_2=curl_exec ($ch);
curl_close ($ch);
$fp = fopen(Mage::getBaseDir('media') . DS . 'import/'.$filename_2,'w');
fwrite($fp, $rawdata_2);
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_3=curl_exec ($ch);
curl_close ($ch);
$fp = fopen(Mage::getBaseDir('media') . DS . 'import/'.$filename_3,'w');
fwrite($fp, $rawdata_3);
fclose($fp);

$product->addImageToMediaGallery(Mage::getBaseDir('media') . DS . 'import/' .  $filename_1, array('image', 'small_image','thumbnail'), false, false);
$product->addImageToMediaGallery(Mage::getBaseDir('media') . DS . 'import/' .  $filename_2, array('image', 'small_image','thumbnail'), false, false);
$product->addImageToMediaGallery(Mage::getBaseDir('media') . DS . 'import/' .  $filename_3, array('image', 'small_image','thumbnail'), false, false);

Next time, I would just post your full question the first time around. But really, why not make the curl fetch in a function and then use meze's answer and call that function inside the loop? — Jim
– Jim, Commented Feb 18, 2011 at 15:21
this also seems like a good candidate for codereview.stackexchange.com — Greg Buehler
– Greg Buehler, Commented Feb 18, 2011 at 16:42

meze · Accepted Answer · 2011-02-18 16:40:06Z

5

You could use loops:

$images = array();
for ($i = 1; $i <= 2; $i++) {
   $images[] = $value->getElementsByTagName("Image" . $i)->item(0)->nodeValue;
}

// and then you can get an image via $images[0], $images[1] and so on

All code can be rewritten as Brad F Jacobs suggested:

function downloadAndSave($image) {
    $filename = basename($image);
    $ch = curl_init ($image);
    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);
    return $filename;
}
// here you should have another loop, suppose foreach ($products as $product) {
    for ($i = 1; $i <= 2; $i++) {
       $filename = downloadAndSave($value->getElementsByTagName("Image" . $i)->item(0)->nodeValue);
       $product->addImageToMediaGallery(Mage::getBaseDir('media') . DS . 'import/' .  $filename, array('image', 'small_image','thumbnail'), false, false);
    }
// end of foreach }

edited Feb 18, 2011 at 16:40

answered Feb 18, 2011 at 14:58

meze

15.1k4 gold badges49 silver badges53 bronze badges

Sign up to request clarification or add additional context in comments.

10 Comments

deceze Over a year ago

Either there are only so many ways to do it, or we're psychically linked. ;)

terrid25 Over a year ago

That works great, but i also have further code that is dependent on the $images_x variable, I will update my code:

Jim Over a year ago

Pretty much what I had for my answer :)

terrid25 Over a year ago

@meze thanks for that. That kind of works. My script is looping throguh an XML file, to insert records to a DB. But the code you provided, only seems to find the images for the first product in the XML file

meze Over a year ago

@terrid25 did your code find images for all products? i just noticed did you have $fp for the $filename_2?

|

deceze · Accepted Answer · 2011-02-18 14:59:29Z

1

$images = array();
for ($x = 1; $x <= 3; $x++) {
    $images[] = $value->getElementsByTagName("Image$x")->item(0)->nodeValue;
}

answered Feb 18, 2011 at 14:59

deceze♦

525k89 gold badges806 silver badges954 bronze badges

Comments

Sam Hogarth · Accepted Answer · 2011-02-18 14:59:29Z

1

You could bundle this into a function:

$image = getImage("Image1");

function getImage($path)
{
  $image_raw = $value->getElementsByTagName($path);
  return $image_raw->item(0)->nodeValue;
}

Furthermore, you could read the resource names from a file (if this is appropriate in the context you wish to use this in), returning an array of values in a foreach loop:

Pseudocode:

$imgArray = array();

foreach($pathNames as $path)
{
   $imgArray[] = getImage($path);
}

answered Feb 18, 2011 at 14:59

Sam Hogarth

5854 silver badges13 bronze badges

Comments

Cfreak · Accepted Answer · 2011-02-18 14:59:33Z

1

Use a loop?

$max_ids = 5; // or whatever
$images = array();

for( $i=1; $i <= $max_ids; $i++ ) {
    $images[$i]['tag'] = $value->getElementsByTagName("Image$i");
    $images[$i]['value'] = $image[$i]['tag']->item(0)->nodeValue;
}

Also creates a data structure for your images.

answered Feb 18, 2011 at 14:59

Cfreak

19.4k6 gold badges52 silver badges64 bronze badges

Comments

GolezTrol · Accepted Answer · 2011-02-18 15:00:00Z

1

Best use arrays. This will give you an array of images:

for ($i = 1; $i < 3; $i++)
{
  $imageElement = $value->getElementsByTagName("Image".$i);
  $imageElements[] = $imageElement;
  $images[] = $imageElement->item(0)->nodeValue;
}

answered Feb 18, 2011 at 15:00

GolezTrol

116k19 gold badges186 silver badges215 bronze badges

Comments

Joseph Le Brech · Accepted Answer · 2011-02-18 14:58:25Z

0

create a function to combine the two lines.

answered Feb 18, 2011 at 14:58

Joseph Le Brech

6,64911 gold badges54 silver badges84 bronze badges

Collectives™ on Stack Overflow

PHP - simple refactor

6 Answers 6

10 Comments

Comments

Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

6 Answers 6

10 Comments

Comments

Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related