Welcome Guest, Not a member yet? Register   Sign In
feedback on code (upload library)
#1

[eluser]brian88[/eluser]
Here Im allowing a user to upload a max of 5 images and stores the image name in a database. Im most likely going to bump it up to 10 images. As you can see I have a lot of repeated code and its going to double when I go from uploading 5 images to 10 images.

Everything works, but Im looking for feedback to make this code shorter and more simple for the controller and the view.


heres what the database looks like...... if no picture is uploaded then the default is "nopic"

id | name | image1 | image2 | image3 | image4 | image5
----------------------------------------------------------------------------------------------------------
1 | Dodge | dodge/pic1.png | dodge/pic2.png | dodge/pic3.png | dodge/pic4.png | nopic
2 | Ford | ford/pic1.png | ford/pic2.png | nopic | nopic | nopic
*/

controller
Code:
function index() {
   // get all trucks from database
   $data['trucks'] = $this->main_mod->getTrucks();

   // if user presses submit button
  if($this->input->post()){

   // get the name of the truck
   $db_data['name'] = $this->input->post('name');

   // format the truck name to be a folder name
   $folder = str_replace( " ", "", strtolower($this->input->post('name')) );

   // make a folder based off the name of the truck
   mkdir('assets/images/'.$folder, 0777);

   // config for file uploads
   $config['upload_path'] = FCPATH . 'assets/images/' . $folder;
   $config['allowed_types'] = 'gif|jpg|png|jpeg';
   $config['max_size'] = '0'; //1000 = 1MB
   $config['max_width']  = '0';
   $config['max_height']  = '0';
   $this->load->library('upload', $config);

   // check to see if user selected a file 1
   if ( !empty($_FILES['file1']['name']) ){
    // upload file 1
    if($this->upload->do_upload('file1')){
     $upload_data = $this->upload->data();
     $db_data['image1'] = $folder.'/'.$upload_data['file_name'];
    }else{
     echo $this->upload->display_errors();
    }
   }

   // check to see if user selected a file 2
   if ( !empty($_FILES['file2']['name']) ){
    // upload file 2
    if($this->upload->do_upload('file2')){
     $upload_data = $this->upload->data();
     $db_data['image2'] = $folder.'/'.$upload_data['file_name'];
    }else{
     echo $this->upload->display_errors();
    }
   }

   // check to see if user selected a file 3
   if ( !empty($_FILES['file3']['name']) ){
    // upload file 3
    if($this->upload->do_upload('file3')){
     $upload_data = $this->upload->data();
     $db_data['image3'] = $folder.'/'.$upload_data['file_name'];
    
    }else{
     echo $this->upload->display_errors();
    }
   }

   // check to see if user selected a file 4
   if ( !empty($_FILES['file4']['name']) ){
    // upload file 4
    if($this->upload->do_upload('file4')){
     $upload_data = $this->upload->data();
     $data['image4'] = $folder.'/'.$upload_data['file_name'];
    }else{
     echo $this->upload->display_errors();
    }
   }

   // check to see if user selected a file 5
   if ( !empty($_FILES['file5']['name']) ){
    // upload file 5
    if($this->upload->do_upload('file5')){
     $upload_data = $this->upload->data();
     $db_data['image5'] = $folder.'/'.$upload_data['file_name'];
    }else{
     echo $this->upload->display_errors();
    }
   }

   // add to database
   $this->main_mod->addTruck($db_data);

   redirect('/');
  }else{
   // views
   $data['content'] = 'main/post';
   $this->load->view('templates/main', $data);
  }
  
} // end function

view
Code:
<div id='content'>

  &lt;?php foreach($trucks as $t): ?&gt;

   <p>&lt;?php echo $t->name; ?&gt;</p>

  //"nopic" is the default name if user does not upload a picture to database.

  //if image does not equal "nopic", then display image.
  &lt;?php if( ($t->image1) != 'nopic' ): ?&gt;

   <img src="&lt;?php echo base_url('assets/images/').'/'.$t-&gt;image1; ?&gt;" width="50" alt=""><br />
  &lt;?php endif; ?&gt;

  // if image does not equal "nopic", then display image.
  &lt;?php if( ($t->image2) != 'nopic' ): ?&gt;

   <img src="&lt;?php echo base_url('assets/images/').'/'.$t-&gt;image2; ?&gt;" width="30" alt="">
  &lt;?php endif; ?&gt;

  // if image does not equal "nopic", then display image.
  &lt;?php if( ($t->image3) != 'nopic' ): ?&gt;

   <img src="&lt;?php echo base_url('assets/images/').'/'.$t-&gt;image3; ?&gt;" width="30" alt="">
  &lt;?php endif; ?&gt;

  // if image does not equal "nopic", then display image.
  &lt;?php if( ($t->image4) != 'nopic' ): ?&gt;

   <img src="&lt;?php echo base_url('assets/images/').'/'.$t-&gt;image4; ?&gt;" width="30" alt="">
  &lt;?php endif; ?&gt;

  // if image does not equal "nopic", then display image.
  &lt;?php if( ($t->image5) != 'nopic' ): ?&gt;

   <img src="&lt;?php echo base_url('assets/images/').'/'.$t-&gt;image5; ?&gt;" width="30" alt="">
  &lt;?php endif; ?&gt;

  &lt;?php endforeach; ?&gt;


// show upload errors
&lt;?php if(isset($error)){echo $error;}; ?&gt;

// form to upload images
   &lt;form action="&lt;?php echo base_url('upload'); ?&gt;" method="post" accept-charset="utf-8" enctype="multipart/form-data"&gt;
    &lt;input type="text" name="name"&gt;&lt;br />
    &lt;input type="file" name="file1"&gt;&lt;br />
    &lt;input type="file" name="file2"&gt;&lt;br />
    &lt;input type="file" name="file3"&gt;&lt;br />
    &lt;input type="file" name="file4"&gt;&lt;br />
    &lt;input type="file" name="file5"&gt;&lt;br />
    &lt;input type="submit" name="submit" value="submit"&gt;
   &lt;/form&gt;

  </div>&lt;!-- #Content --&gt;
#2

[eluser]brian88[/eluser]
any way to make this simpler code?
#3

[eluser]craig.hoog[/eluser]
At the very least you could probably do a foreach loop and replace the number in each instance so that it loops through your files instead of doing this so many times.
#4

[eluser]boltsabre[/eluser]
Set a variable at the start of your controller for the number of images (5 or 10 or whatever you want). Then where ever you find yourself repeating code for the images just use a loop using the afore mentioned variable as your total number of iterations

Code:
// controller
$num_of_images = 10;
//and pass it to your view as well!
$data['num_of_images '] = $num_of_images;
...
...
...
for ($i=1; $i<=$num_of_images ; $i++){
if ( !empty($_FILES["file".$i]['name']) ){
    // upload file 1
    if($this->upload->do_upload("file".$i)){
     $upload_data = $this->upload->data();
     $db_data["image".$i] = $folder.'/'.$upload_data['file_name'];
    }else{
     echo $this->upload->display_errors();
    }
   }
}

Code:
// your view
for ($i=1; $i<=$num_of_images ; $i++){
echo "&lt;input type='file' name='file$i&gt;&lt;br /&gt;
}

You get the idea! This way you just have to change $num_of_images at the start of your controller and it changes everything else for you because you've got it all inside loops. (not tested, you may have to play around with the concatenation aspects of it, but the logic is sound).

I haven't looked at the CI image uploader library, so I'm not sure how much of the security risks it covers, but you should research it. File uploads leave MASSIVE security holes. You should be doing such things as giving the images random image names, using PHP to "redraw" the image (it's possible to upload a file that an image extension but is actually a php or shell script, by redrawing the image, it checks against this. Then there is the double extension hack, users uploading .htaccess files to your image folders and much more - look into it otherwise you could find your site crashing - there are soooo many "out of the box" hacking programs that bored kiddies are using to crash sites just for the fun of it that you really cannot afford to not look into this. Just google "php file upload security" and you'll get some good hits!

Good luck with it!
#5

[eluser]PhilTem[/eluser]
You might also remove all fields named "image<int>" and make a new column called "images" where you store all images in a json_encode'd or serialized array. Then the looping is even easier than mentioned by @boltsabre.

May a row be updated every once in a while or is it created once and never changed? With the latter case it's pretty easy to change your code, otherwise it's a bit more work, but not too much either.
Scream if you need help Wink
#6

[eluser]brian88[/eluser]
Thanks. both are good answers. I did boltsabre's way. And now I wanna do PhilTem's way also.

I made a new column "images"

So now I can store all the trucks into this column with a serialized array?

I showed all my code above but heres the model and updated controller code.
Where can i serialize the data then unserialize for the view?

model
Code:
function addTruck($data){
  //$d = serialize($data); // dont work
  $insert = $this->db->insert('trucks', $data);
  return $insert;
} // end function

controller updated code
Code:
function index() {
   // get all trucks from database
   $data['trucks'] = $this->main_mod->getTrucks();

   $data['num_of_images'] = 5;

   // if user presses submit button
  if($this->input->post()){
   // get the name of the truck
   $db_data['name'] = $this->input->post('name');
   // format the truck name to be a folder name
   $folder = str_replace( " ", "", strtolower($this->input->post('name')) );
  
   // if the name of the truck folder does not currently exist.
   if(!is_dir('assets/images/'.$folder)){

    // make a folder based off the name of the truck
    mkdir('assets/images/'.$folder, 0777);

    // config for file uploads
    $config['upload_path'] = FCPATH . 'assets/images/' . $folder;
    $config['allowed_types'] = 'gif|jpg|png|jpeg';
    $config['max_size'] = '0'; //1000 = 1MB
    $config['max_width']  = '0';
    $config['max_height']  = '0';
    $this->load->library('upload', $config);
    
    // upload users submitted images
    for ($i=1; $i<=$data['num_of_images']; $i++){
     // if user uploaded an image
     if ( !empty($_FILES["file" . $i]['name']) ){
      // upload file
      if($this->upload->do_upload("file".$i)){
       $upload_data = $this->upload->data();
       $db_data["images"] = $folder.'/'.$upload_data['file_name'];
      }else{ // error
       echo $this->upload->display_errors();
      }
     }
    }

    // add to database
    $this->main_mod->addTruck($db_data);

    // refresh the page to see view new data added.
    redirect('/');

    // Debug
    foreach($db_data as $d){
     echo serialize($d) . '<br /><br />';
     // uploaded 1 photo....it returns
     // s:9:"truckname";
     // s:18:"truckname/logo.jpg";
    }
   }else{
    // The truck name folder exist. Need a new name for the folder.
    echo 'Error: This truck name already exist. Choose a different name.';
   }
  }else{
   // views
   $data['content'] = 'main/post';
   $this->load->view('templates/main', $data);
  }
  
} // end function

I tried
Code:
serialize( $db_data["images"] ) = $folder.'/'.$upload_data['file_name'];
But that fails.
#7

[eluser]brian88[/eluser]
Also I added the loop in the view to make is more simple. It works for the form inputs at the bottom. But my for loop isnt working inside my foreach loops

I get an error "Undefined property: stdClass::$image"

Code:
// this line isnt working
&lt;?php if( $t->image . $i != 'nopic' ): ?&gt;

// if you hard code it, it does work.
&lt;?php if( $t->image1 != 'nopic' ): ?&gt;

view
Code:
<div id='content'>

  &lt;?php foreach($trucks as $t): ?&gt;

   <p>&lt;?php echo $t->name; ?&gt;</p>

  &lt;?php for ($i=1; $i<=$num_of_images ; $i++): ?&gt;


   &lt;?php if( $t->image . $i != 'nopic' ): ?&gt;
    <img src="&lt;?php echo base_url('assets/images/').'/'.$t-&gt;image. $i; ?&gt;" width="50" alt=""><br />
   &lt;?php endif; ?&gt;

  &lt;?php endfor; ?&gt;

  &lt;?php endforeach; ?&gt;

   &lt;?php if(isset($error)){echo $error;}; ?&gt;

   &lt;form action="&lt;?php echo base_url(); ?&gt;" method="post" accept-charset="utf-8" enctype="multipart/form-data"&gt;
    &lt;input type="text" name="name"&gt;&lt;br />
    &lt;?php for ($i=1; $i<=$num_of_images ; $i++): ?&gt;
     &lt;input type='file' name='file&lt;?php echo $i; ?&gt;'&gt;&lt;br />
    &lt;?php endfor; ?&gt;
    &lt;input type="submit" name="submit" value="submit"&gt;
   &lt;/form&gt;

  </div>&lt;!-- #Content --&gt;
#8

[eluser]Sanjay Sarvaiya[/eluser]
[quote author="brian88" date="1337196196"]Thanks. both are good answers. I did boltsabre's way. And now I wanna do PhilTem's way also.

I made a new column "images"

So now I can store all the trucks into this column with a serialized array?

I showed all my code above but heres the model and updated controller code.
Where can i serialize the data then unserialize for the view?

model
Code:
function addTruck($data){
  //$d = serialize($data); // dont work
  $insert = $this->db->insert('trucks', $data);
  return $insert;
} // end function

controller updated code
Code:
function index() {
   // get all trucks from database
   $data['trucks'] = $this->main_mod->getTrucks();

   $data['num_of_images'] = 5;

   // if user presses submit button
  if($this->input->post()){
   // get the name of the truck
   $db_data['name'] = $this->input->post('name');
   // format the truck name to be a folder name
   $folder = str_replace( " ", "", strtolower($this->input->post('name')) );
  
   // if the name of the truck folder does not currently exist.
   if(!is_dir('assets/images/'.$folder)){

    // make a folder based off the name of the truck
    mkdir('assets/images/'.$folder, 0777);

    // config for file uploads
    $config['upload_path'] = FCPATH . 'assets/images/' . $folder;
    $config['allowed_types'] = 'gif|jpg|png|jpeg';
    $config['max_size'] = '0'; //1000 = 1MB
    $config['max_width']  = '0';
    $config['max_height']  = '0';
    $this->load->library('upload', $config);
    
    // upload users submitted images
    for ($i=1; $i<=$data['num_of_images']; $i++){
     // if user uploaded an image
     if ( !empty($_FILES["file" . $i]['name']) ){
      // upload file
      if($this->upload->do_upload("file".$i)){
       $upload_data = $this->upload->data();
       $db_data["images"] = $folder.'/'.$upload_data['file_name'];
      }else{ // error
       echo $this->upload->display_errors();
      }
     }
    }

    // add to database
    $this->main_mod->addTruck($db_data);

    // refresh the page to see view new data added.
    redirect('/');

    // Debug
    foreach($db_data as $d){
     echo serialize($d) . '<br /><br />';
     // uploaded 1 photo....it returns
     // s:9:"truckname";
     // s:18:"truckname/logo.jpg";
    }
   }else{
    // The truck name folder exist. Need a new name for the folder.
    echo 'Error: This truck name already exist. Choose a different name.';
   }
  }else{
   // views
   $data['content'] = 'main/post';
   $this->load->view('templates/main', $data);
  }
  
} // end function

I tried
Code:
serialize( $db_data["images"] ) = $folder.'/'.$upload_data['file_name'];
But that fails.[/quote]

Hey dude store file name in image array like this.
Code:
$db_data["images"][] = $folder.'/'.$upload_data['file_name'];
#9

[eluser]brian88[/eluser]
Thanks. That helps. So now I get an error when inserting into the database.

Code:
Unknown column 'Array' in 'field list'

INSERT INTO `trucks` (`name`, `images`) VALUES ('dodge', Array)

So how or where do I serialize the array so the database accepts it?

Code:
// didnt work, error - Cannot use [] for reading
serialize( $db_data["images"][] ) = $folder.'/'.$upload_data['file_name'];

//also tried this but same database error as above.
serialize($db_data["images"]);
$this->main_mod->addTruck($db_data);
#10

[eluser]Sanjay Sarvaiya[/eluser]
I get bug and fixed with this.

Code:
$data["images"] = serialize( $db_data["images"]);
$this->main_mod->addTruck($data);


that may helps.




Theme © iAndrew 2016 - Forum software by © MyBB