Advice on doing this better, faster and neater (form and image upload)

[eluser]bugboy[/eluser]
I have a form that allows the user to upload an image as well.

The image isn't a required element so can be left out but its there if the user wants to have one.

Now this is my controller I feel its very messy so i'm looking for advice on how i can best do it.

Code:

function add_news()
    {
    $error = '';
    $image = '0';
    $insert_me = FALSE;    
    $data['module'] = $this->_module;
    $data['controller'] =$this->_controller;
    $data['area']= $this->_area;
    
    $data['page'] = $this->_controller.'/base';
    $data['content'] = $this->_controller.'/add_news';

        
    // validation area
    $rules['title'] = "trim|required|xss_clean";
    $rules['tag'] = "trim|required|xss_clean";
    $rules['dscpn'] = "trim|required|xss_clean";
    $rules['text'] = "trim|required|xss_clean";
    $rules['keyword'] = "trim|xss_clean";
    $this->validation->set_rules($rules);
    $this->validation->run();    
    $fields['title'] = 'title';
    $fields['tag'] = 'tag';
    $fields['dscpn'] = 'description';
    $fields['text'] = 'text';
    $fields['keyword']  = 'keyword';
    $fields['day'] = 'day';
    $fields['month'] = 'month';
    $fields['year'] = 'year';
    $this->validation->set_fields($fields);
    
    
    $data['heading'] = 'Add a News artical';
    
    $tag = $this->input->post('tag');
    if(!empty($tag)){
    $checkTag = $this->admin->checkNewsTag($tag);
    }else{
    $checkTag = FALSE;
    }

    if ($this->validation->run() == FALSE || $checkTag == TRUE)
        {
            if($checkTag == TRUE){
            $data['tag_message'] = '<div class="error">Sorry this tag is already in use</div>';
            }
            $data['image_error'] = $error;
            $this->load->vars($data);
            $this->load->view($this->_container);
        }else{
            //if validation  run success for (input,texarea,radio,select,etc..)  then check for upload validation below
            if($_FILES['image']['name'] != ''){
                if (!$this->upload->do_upload('image')){
                    //fail upload and set String validation error
                    $error = array($this->upload->display_errors());
                    $data['image_error'] = $error;
                    $this->load->vars($data);
                    $this->load->view($this->_container);
                }else{
                    //upload done and work now resize me
                    $upload_data = $this->upload->data();
                    $image = $upload_data["file_name"];
                    // run resize and thumbnail
                    $this->image_misc->resizeImage($image, './uploaded_images/', 500, 500);
                    $this->image_misc->resizeImage($image, './uploaded_images/', 75, 50, TRUE);                
                
                    $insert_me = TRUE;
                }
            }else{
                $insert_me = TRUE;
            }
        }
    
        if($insert_me == TRUE){
            
            $date = $this->input->post('year').'-'.$this->input->post('month').'-'.$this->input->post('day');
            $addUnderScore = str_replace(" ", "_", trim($this->input->post('tag')));
            $insert = array (
                'title' => $this->input->post('title'),
                'keyword' => $this->input->post('keyword'),
                'dscpn' => ascii_to_entities($this->input->post('dscpn')),
                'tag' => $addUnderScore,
                'text' => ascii_to_entities($this->input->post('text')),
                'author' => $this->db_session->userdata('user_name'),
                'image' => $image,
                'date' => $date
                );
                $insert = $this->admin->addNews($insert);
                redirect($this->_module.'/console', 'refresh');
        }
    }

It works a treat but any advice would be greatly appreciated

[eluser]Crimp[/eluser]
The first thing I would do it to move all db logic to a model. That would clean up your controllers a lot. The second thing I tend to do to keep the actual controller methods easy to read is to move lengthy validation sets to a private function. In your case, that would drop _news_validation() to the bottom. I would also move the file upload to its own function. With all that done, you could easily glance at add_news() and figure it out in an instant.

[eluser]bugboy[/eluser]
Wicked cheers some of it makes sense some doesn't

What would you class as db logic on there as i have my model (admin) doing the inserting and such?

So make news validation a private function i didn't know you could do this with validation what would this look like, how would you put the data in and out of it?

So with the upload function i only call it when i need it if its needed pass it the post file data and let it return a array if its completed or a true/ false / null thing?

This is great advice. Sorry if i sound thick though

[eluser]bugboy[/eluser]
when moving the vaildation sets to a function would i just run that function everytime the controller is called and get the function to return the

$this->validation->run();

TRUE or FALSE

so something like this

Code:

function _news_validaton()
{
    // validation area
    $rules['title'] = "trim|required|xss_clean";
    $rules['tag'] = "trim|required|xss_clean";
    $rules['dscpn'] = "trim|required|xss_clean";
    $rules['text'] = "trim|required|xss_clean";
    $rules['keyword'] = "trim|xss_clean";
    $this->validation->set_rules($rules);  
    $fields['title'] = 'title';
    $fields['tag'] = 'tag';
    $fields['dscpn'] = 'description';
    $fields['text'] = 'text';
    $fields['keyword']  = 'keyword';
    $fields['day'] = 'day';
    $fields['month'] = 'month';
    $fields['year'] = 'year';
    $this->validation->set_fields($fields);

    return $this->validation->run();
}

then in the controller do this

Code:

$validate = $this->_news_validaton()

if ($validate== FALSE )
{
//carry on with code
}

EDITED: yep that works a treat. plu it cuts down on the amount of of code so i can run it for the edit part as well.

[eluser]Référencement Google[/eluser]
You also can group the validation strings into a variable while it's repetitive, that would make something like this:

Code:

// validation area
$vstring = "trim|required|xss_clean";

$rules['title'] = $vstring;
$rules['tag'] = $vstring;
$rules['dscpn'] = $vstring;
$rules['text'] = $vstring;

Also, I just don't like to code like that, but I think it would even be possible to make:

Code:

$vstring = "trim|required|xss_clean";
$rules['title'] = $rules['tag'] = $rules['dscpn'] = $rules['text'] = $vstring;

And even:

Code:

$vstring = "trim|required|xss_clean";
$rules = array('title', 'tag', 'dscpn', 'text') = $vstring; // Not tested though...

[eluser]Référencement Google[/eluser]
[quote author="bugboy" date="1210793351"]

Code:

$validate = $this->_news_validaton()

if ($validate== FALSE )
{
//carry on with code
}

[/quote]

You simply can do:

Code:

if ($this->_news_validaton())
{
   //carry on with code
}

[eluser]bugboy[/eluser]
sweet i'll do that

what can i do about my image upload part. I'm trying to rewrite my function to make work but i can't seem to work it out.

[eluser]bugboy[/eluser]
[quote author="elitemedia" date="1210795410"][quote author="bugboy" date="1210793351"]

Code:

$validate = $this->_news_validaton()

if ($validate== FALSE )
{
//carry on with code
}

[/quote]

You simply can do:

Code:

if ($this->_news_validaton())
{
   //carry on with code
}

[/quote]


Yeah i worked that out just as i got your reply. Cheers