Codeigniter Controller for work orders

The name of the pictureThe name of the pictureThe name of the pictureClash Royale CLAN TAG#URR8PPP





.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;







up vote
2
down vote

favorite












I'm working on a project and one of my controllers is getting quite large and hard to maintain or add to. I'm having trouble deciding what belongs in the controller and what I can move to the model. What can I change to improve this?



class Job_update extends MY_Controller


function __construct()

parent::__construct();
$this->load->helper('form');
$this->load->library('email');
$this->load->helper('html');
$this->load->model('jobs_model');

/**
* Reassigns a tradie to the work order sending appropriate emails out
* @param int $job_id
*/
public function reassign_tradie($job_id)

$this->load->model('send_emails_model');
$this->load->model('tradie_model');
$this->load->model('notes_model');
//get tradie ids via get or post
if ($this->input->get("tradie"))
$tradie_id = $this->input->get("tradie");
$prev_tradie = $this->input->get("prev_tradie");
else
$tradie_id = $this->input->post("tradie");
$prev_tradie = $this->input->post("prev_tradie");


if ($tradie_id == $prev_tradie)
$this->session->set_flashdata('error', "That tradesman is already assigned to this job");
redirect('job_update/job_id/'.$job_id);
return;


//retrieve data for both tradies
$data = $this->tradie_model->get($tradie_id); //new tradie
$prev_tradie_data = $this->tradie_model->get($prev_tradie);//prev tradie

//update the status to pending and set the new tradie
$this->jobs_model->update($job_id, array(
"status" => "Pending",
"tradie" => $tradie_id,
"job_tradie_response" => 0
));

//add a note to the job documenting the change
$job_note_message = "Tradesman has been changed from " . $prev_tradie_data->tradie_firstname .
" to ". $data->tradie_firstname . " Reason: " . $this->input->post("reason");
$this->notes_model->insert($job_id, $job_note_message);

//select the new job data
$job_data = $this->jobs_model->with_site_location()->with_tradie()->get($job_id);

//send the tradie an email of the work order
$tradie_email = $this->jobs_model->build_tradie_email($job_id, $job_data);
$this->send_emails_model->insert($tradie_email);

//if the prev tradie didn't decline the job we need to send a cancellation email
if ($job_data->status != "Declined")
$this->load->model('tradie_model');

$tradie_data = $this->tradie_model->get($prev_tradie);

$cancelled_email = $this->jobs_model->build_job_cancelled_email($job_id, $tradie_data);
$this->send_emails_model->insert($cancelled_email);


//redirect
$this->session->set_flashdata('msg', "Tradesman has been successfully changed");
redirect('job_update/job_id/'.$job_id);


/**
* Resends the work order to the assigned plumber also updates the status to pending
* @param int $job_id
*/
public function resend_work_order($job_id)

$this->load->model('notes_model');
$this->load->model('send_emails_model');
//updates status of job
$job_update = array("status" => "Pending", "job_tradie_response" => 0);
$this->jobs_model->update($job_update, $job_id);

//Sends email to tradesman
$job_data = $this->jobs_model->with_site_location()->with_tradie()->get($job_id);
$result = $this->jobs_model->build_tradie_email($job_id, $job_data);

$this->send_emails_model->insert($result);

//addes note to system talking about change
$data = array(
'note' => "Job has been resent to the plumber. Status changed to Pending",
'notes_job_id' => $job_id,
'datestamp' => date("Y-m-d H:i:s"),
'user_id' => $this->session->userdata('username')
);
$this->notes_model->insert($data);

$this->session->set_flashdata('msg', 'Resent Work Order to '. $job_data->tradie->tradie_firstname);
redirect('job_update/job_id/'.$job_id);


/**
* Inserts a new note from post data
* @param int $job_id
*/
public function job_add_note($job_id)

$this->_add_note($job_id, $this->input->post('job_notes'));

$this->session->set_flashdata('msg', 'Job Note Added Successfully');
redirect('job_update/job_id/'.$job_id);


/**
* Loads the photos page for a certain job
* @param int $job_id
*/
public function photos($job_id)

$data = array('job_id' => $job_id);
$this->load->view('job_photos_view', $data);



public function set_flash_for_bookin($job_id)

$this->session->set_flashdata('msg', 'Date/Time has been successfully set');
redirect('job_update/job_id/'.$job_id);


/**
* Saves the work order details (checkboxes, job_description)
* @param int $job_id
*/
public function submit_work_order($job_id)

$data = $this->update_checkboxes($job_id); //get all checkbox data
$this->jobs_model->update($data, $job_id); //save to db
$this->session->set_flashdata('msg', 'Work Order Updated Successfully');
redirect('job_update/job_id/'.$job_id);



/**
* Resends the customer confirmation email
* @param string (email) $c_email
* @param int $job_id
*/
public function resend_confirmation_email($c_email, $job_id)

$this->load->model('send_emails_model');
$this->job_update_model->update_send_email(array("email_sent" => 0), urldecode($c_email));

$this->session->set_flashdata('msg', 'Email Successfully Sent');
redirect('job_update/job_id/'.$job_id);


/**
* Sets the date for a checkbox
* @param string $post_data
* @param date $current_date_in_db
* @param bool (0,1) $is_checked
* @return date string format YYYY-MM-DD
*/
private function checkbox_helper($post_data, $current_date_in_db, $is_checked)
if($post_data == false)
return "0000-00-00";
else if($post_data != false && $is_checked === "0")
return date("Y-m-d");
else
return $current_date_in_db;


/**
* returns an array with checkbox data
* @param int $job_id
* @return array
*/
public function update_checkboxes($job_id)
$job_data = $this->jobs_model->get($job_id);

$current_date = date('Y-m-d');
$current_time = date('H:i:s');

//if 1 box is checked if 0 box is not checked
$data = array(
'box_customer_called' => ($this->input->post('checkbox1') == false ? '0' : '1'),
'box_customer_called_date' => $this->checkbox_helper($this->input->post('checkbox1'), $job_data->box_customer_called_date, $job_data->box_customer_called),
'box_photos_reviewed' => ($this->input->post('checkbox2') == false ? '0' : '1'),
'box_photos_received_date' => $this->checkbox_helper($this->input->post('checkbox2'), $job_data->box_photos_received_date, $job_data->box_photos_reviewed),
'box_photos_received' => ($this->input->post('checkbox6') == false ? '0' : '1'),
'box_photos_received_date' => $this->checkbox_helper($this->input->post('checkbox6'), $job_data->box_photos_received_date, $job_data->box_photos_received),
'box_completed' => ($this->input->post('checkbox7') == false ? '0' : '1'),
'box_completed_date' => $this->checkbox_helper($this->input->post('checkbox7'), $job_data->box_completed_date, $job_data->box_completed),
'box_completion_certificate' => ($this->input->post('checkbox3') == false ? '0' : '1'),
'box_completion_certificate_date' => $this->checkbox_helper($this->input->post('checkbox3'), $job_data->box_completion_certificate_date, $job_data->box_completion_certificate),
'box_invoiced' => ($this->input->post('checkbox4') == false ? '0' : '1'),
'box_invoiced_date' => $this->checkbox_helper($this->input->post('checkbox4'), $job_data->box_invoiced_date, $job_data->box_invoiced),
'box_warranty' => ($this->input->post('checkbox5') == false ? '0' : '1'),
'box_warranty_date' => $this->checkbox_helper($this->input->post('checkbox5'), $job_data->box_warranty_date, $job_data->box_warranty),
'job_description' => $this->input->post('job_description_new'),
);

//if the job was completed and is now unticked change the status
if($job_data->box_completed == 0 && $this->input->post('checkbox7') == "Completed")
$data['status'] = "Completed";

if($job_data->box_completed == 1 && $this->input->post('checkbox7') != "Completed")
$data['status'] = "Accepted";

return $data;


/**
* Inserts an email into the send que
* @param string $message
* @param string $email
* @param string $name
* @param string $email_subject
*/
public function email($message, $email, $name, $email_subject)

$this->load->model('send_emails_model');

$data = array('message' => $message,
'email' => $email,
'name' => $name,
'email_subject' => $email_subject);

$this->send_emails_model->insert($data);


/**
* Uploads a photo to the job directotry
* @param int $job_id
*/
public function upload_images($job_id)


$name_array = array();

if (!is_dir('assets/job_photos/'.add_prefix($job_id))) //create directory is it doesn't exist
mkdir('./assets/job_photos/'.add_prefix($job_id), 0777, true);
$dir_exist = false; // dir not exist


$count = count($_FILES['userfile']['size']);
foreach ($_FILES as $key => $value)
for ($s=0; $s<=$count-1; $s++)
$_FILES['userfile']['name']=$value['name'][$s];
$_FILES['userfile']['type'] = $value['type'][$s];
$_FILES['userfile']['tmp_name'] = $value['tmp_name'][$s];
$_FILES['userfile']['error'] = $value['error'][$s];
$_FILES['userfile']['size'] = $value['size'][$s];
$config['upload_path'] = './assets/job_photos/'.add_prefix($job_id).'/';
$config['allowed_types'] = 'gif

redirect(site_url('job_update/job_id/'.$job_id));


public function job_id($job_id)

$this->load->model('user_model');
$this->load->model('tradie_model');
$this->load->model('send_emails_model');
$this->load->model('notes_model');

//gather all data required for display
$job_data = $this->jobs_model->with_site_location()->with_tradie()->with_job_notes()->get($job_id);

$all_users['users'] = $this->user_model->select_all_users();
$tradies['tradies'] = $this->tradie_model->get_all();

$email = $this->send_emails_model->where('email', $job_data->site_location->tenant_email)->get();


//if job isn't found job data will be empty so we load a blank page and return
if (empty($job_data))
$this->headerlib->load_header();
return;


//change all 1's to checked for view
foreach ($job_data as $key => &$value)
//filter thourgh all table coloumns to find the boxes coloumns
if(strpos($key, 'box') !== false && strpos($key, 'date') === false)
if($value == 1)
$value = "checked";



$data = array_merge((array)$job_data, (array)$all_users, (array)$job_emails, (array)$tradies);

$this->headerlib->load_header();
$this->footerlib->load_footer();
$this->load->view('job_update_view', $data);


/**
* Deletes a job
* @param int $job_id job_id you wish to delete
*/
public function delete_job($job_id)

$this->load->model('send_emails_model');
$this->load->model('tradie_model');

if($this->session->userdata('user_team_id') != 40)

$this->session->set_flashdata('error', 'You do not have permission to delete this job');
redirect($_SERVER['HTTP_REFERER']);
return;


$data = $this->jobs_model->with_tradie()->get($job_id);

$tradie_data = $this->tradie_model->get($data->tradie->tradie_id);

$cancelled_email = $this->jobs_model->build_job_cancelled_email($job_id, $tradie_data);
$this->send_emails_model->insert($cancelled_email);

if ($this->jobs_model->delete($job_id))
$this->session->set_flashdata('msg', 'Job has been successfully deleted');
else
$this->session->set_flashdata('error', 'Something went wrong, contact an administrator');


redirect($_SERVER['HTTP_REFERER']);




Thanks for the help!







share|improve this question





















  • Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate.
    – Sam Onela
    Jan 9 at 2:31
















up vote
2
down vote

favorite












I'm working on a project and one of my controllers is getting quite large and hard to maintain or add to. I'm having trouble deciding what belongs in the controller and what I can move to the model. What can I change to improve this?



class Job_update extends MY_Controller


function __construct()

parent::__construct();
$this->load->helper('form');
$this->load->library('email');
$this->load->helper('html');
$this->load->model('jobs_model');

/**
* Reassigns a tradie to the work order sending appropriate emails out
* @param int $job_id
*/
public function reassign_tradie($job_id)

$this->load->model('send_emails_model');
$this->load->model('tradie_model');
$this->load->model('notes_model');
//get tradie ids via get or post
if ($this->input->get("tradie"))
$tradie_id = $this->input->get("tradie");
$prev_tradie = $this->input->get("prev_tradie");
else
$tradie_id = $this->input->post("tradie");
$prev_tradie = $this->input->post("prev_tradie");


if ($tradie_id == $prev_tradie)
$this->session->set_flashdata('error', "That tradesman is already assigned to this job");
redirect('job_update/job_id/'.$job_id);
return;


//retrieve data for both tradies
$data = $this->tradie_model->get($tradie_id); //new tradie
$prev_tradie_data = $this->tradie_model->get($prev_tradie);//prev tradie

//update the status to pending and set the new tradie
$this->jobs_model->update($job_id, array(
"status" => "Pending",
"tradie" => $tradie_id,
"job_tradie_response" => 0
));

//add a note to the job documenting the change
$job_note_message = "Tradesman has been changed from " . $prev_tradie_data->tradie_firstname .
" to ". $data->tradie_firstname . " Reason: " . $this->input->post("reason");
$this->notes_model->insert($job_id, $job_note_message);

//select the new job data
$job_data = $this->jobs_model->with_site_location()->with_tradie()->get($job_id);

//send the tradie an email of the work order
$tradie_email = $this->jobs_model->build_tradie_email($job_id, $job_data);
$this->send_emails_model->insert($tradie_email);

//if the prev tradie didn't decline the job we need to send a cancellation email
if ($job_data->status != "Declined")
$this->load->model('tradie_model');

$tradie_data = $this->tradie_model->get($prev_tradie);

$cancelled_email = $this->jobs_model->build_job_cancelled_email($job_id, $tradie_data);
$this->send_emails_model->insert($cancelled_email);


//redirect
$this->session->set_flashdata('msg', "Tradesman has been successfully changed");
redirect('job_update/job_id/'.$job_id);


/**
* Resends the work order to the assigned plumber also updates the status to pending
* @param int $job_id
*/
public function resend_work_order($job_id)

$this->load->model('notes_model');
$this->load->model('send_emails_model');
//updates status of job
$job_update = array("status" => "Pending", "job_tradie_response" => 0);
$this->jobs_model->update($job_update, $job_id);

//Sends email to tradesman
$job_data = $this->jobs_model->with_site_location()->with_tradie()->get($job_id);
$result = $this->jobs_model->build_tradie_email($job_id, $job_data);

$this->send_emails_model->insert($result);

//addes note to system talking about change
$data = array(
'note' => "Job has been resent to the plumber. Status changed to Pending",
'notes_job_id' => $job_id,
'datestamp' => date("Y-m-d H:i:s"),
'user_id' => $this->session->userdata('username')
);
$this->notes_model->insert($data);

$this->session->set_flashdata('msg', 'Resent Work Order to '. $job_data->tradie->tradie_firstname);
redirect('job_update/job_id/'.$job_id);


/**
* Inserts a new note from post data
* @param int $job_id
*/
public function job_add_note($job_id)

$this->_add_note($job_id, $this->input->post('job_notes'));

$this->session->set_flashdata('msg', 'Job Note Added Successfully');
redirect('job_update/job_id/'.$job_id);


/**
* Loads the photos page for a certain job
* @param int $job_id
*/
public function photos($job_id)

$data = array('job_id' => $job_id);
$this->load->view('job_photos_view', $data);



public function set_flash_for_bookin($job_id)

$this->session->set_flashdata('msg', 'Date/Time has been successfully set');
redirect('job_update/job_id/'.$job_id);


/**
* Saves the work order details (checkboxes, job_description)
* @param int $job_id
*/
public function submit_work_order($job_id)

$data = $this->update_checkboxes($job_id); //get all checkbox data
$this->jobs_model->update($data, $job_id); //save to db
$this->session->set_flashdata('msg', 'Work Order Updated Successfully');
redirect('job_update/job_id/'.$job_id);



/**
* Resends the customer confirmation email
* @param string (email) $c_email
* @param int $job_id
*/
public function resend_confirmation_email($c_email, $job_id)

$this->load->model('send_emails_model');
$this->job_update_model->update_send_email(array("email_sent" => 0), urldecode($c_email));

$this->session->set_flashdata('msg', 'Email Successfully Sent');
redirect('job_update/job_id/'.$job_id);


/**
* Sets the date for a checkbox
* @param string $post_data
* @param date $current_date_in_db
* @param bool (0,1) $is_checked
* @return date string format YYYY-MM-DD
*/
private function checkbox_helper($post_data, $current_date_in_db, $is_checked)
if($post_data == false)
return "0000-00-00";
else if($post_data != false && $is_checked === "0")
return date("Y-m-d");
else
return $current_date_in_db;


/**
* returns an array with checkbox data
* @param int $job_id
* @return array
*/
public function update_checkboxes($job_id)
$job_data = $this->jobs_model->get($job_id);

$current_date = date('Y-m-d');
$current_time = date('H:i:s');

//if 1 box is checked if 0 box is not checked
$data = array(
'box_customer_called' => ($this->input->post('checkbox1') == false ? '0' : '1'),
'box_customer_called_date' => $this->checkbox_helper($this->input->post('checkbox1'), $job_data->box_customer_called_date, $job_data->box_customer_called),
'box_photos_reviewed' => ($this->input->post('checkbox2') == false ? '0' : '1'),
'box_photos_received_date' => $this->checkbox_helper($this->input->post('checkbox2'), $job_data->box_photos_received_date, $job_data->box_photos_reviewed),
'box_photos_received' => ($this->input->post('checkbox6') == false ? '0' : '1'),
'box_photos_received_date' => $this->checkbox_helper($this->input->post('checkbox6'), $job_data->box_photos_received_date, $job_data->box_photos_received),
'box_completed' => ($this->input->post('checkbox7') == false ? '0' : '1'),
'box_completed_date' => $this->checkbox_helper($this->input->post('checkbox7'), $job_data->box_completed_date, $job_data->box_completed),
'box_completion_certificate' => ($this->input->post('checkbox3') == false ? '0' : '1'),
'box_completion_certificate_date' => $this->checkbox_helper($this->input->post('checkbox3'), $job_data->box_completion_certificate_date, $job_data->box_completion_certificate),
'box_invoiced' => ($this->input->post('checkbox4') == false ? '0' : '1'),
'box_invoiced_date' => $this->checkbox_helper($this->input->post('checkbox4'), $job_data->box_invoiced_date, $job_data->box_invoiced),
'box_warranty' => ($this->input->post('checkbox5') == false ? '0' : '1'),
'box_warranty_date' => $this->checkbox_helper($this->input->post('checkbox5'), $job_data->box_warranty_date, $job_data->box_warranty),
'job_description' => $this->input->post('job_description_new'),
);

//if the job was completed and is now unticked change the status
if($job_data->box_completed == 0 && $this->input->post('checkbox7') == "Completed")
$data['status'] = "Completed";

if($job_data->box_completed == 1 && $this->input->post('checkbox7') != "Completed")
$data['status'] = "Accepted";

return $data;


/**
* Inserts an email into the send que
* @param string $message
* @param string $email
* @param string $name
* @param string $email_subject
*/
public function email($message, $email, $name, $email_subject)

$this->load->model('send_emails_model');

$data = array('message' => $message,
'email' => $email,
'name' => $name,
'email_subject' => $email_subject);

$this->send_emails_model->insert($data);


/**
* Uploads a photo to the job directotry
* @param int $job_id
*/
public function upload_images($job_id)


$name_array = array();

if (!is_dir('assets/job_photos/'.add_prefix($job_id))) //create directory is it doesn't exist
mkdir('./assets/job_photos/'.add_prefix($job_id), 0777, true);
$dir_exist = false; // dir not exist


$count = count($_FILES['userfile']['size']);
foreach ($_FILES as $key => $value)
for ($s=0; $s<=$count-1; $s++)
$_FILES['userfile']['name']=$value['name'][$s];
$_FILES['userfile']['type'] = $value['type'][$s];
$_FILES['userfile']['tmp_name'] = $value['tmp_name'][$s];
$_FILES['userfile']['error'] = $value['error'][$s];
$_FILES['userfile']['size'] = $value['size'][$s];
$config['upload_path'] = './assets/job_photos/'.add_prefix($job_id).'/';
$config['allowed_types'] = 'gif

redirect(site_url('job_update/job_id/'.$job_id));


public function job_id($job_id)

$this->load->model('user_model');
$this->load->model('tradie_model');
$this->load->model('send_emails_model');
$this->load->model('notes_model');

//gather all data required for display
$job_data = $this->jobs_model->with_site_location()->with_tradie()->with_job_notes()->get($job_id);

$all_users['users'] = $this->user_model->select_all_users();
$tradies['tradies'] = $this->tradie_model->get_all();

$email = $this->send_emails_model->where('email', $job_data->site_location->tenant_email)->get();


//if job isn't found job data will be empty so we load a blank page and return
if (empty($job_data))
$this->headerlib->load_header();
return;


//change all 1's to checked for view
foreach ($job_data as $key => &$value)
//filter thourgh all table coloumns to find the boxes coloumns
if(strpos($key, 'box') !== false && strpos($key, 'date') === false)
if($value == 1)
$value = "checked";



$data = array_merge((array)$job_data, (array)$all_users, (array)$job_emails, (array)$tradies);

$this->headerlib->load_header();
$this->footerlib->load_footer();
$this->load->view('job_update_view', $data);


/**
* Deletes a job
* @param int $job_id job_id you wish to delete
*/
public function delete_job($job_id)

$this->load->model('send_emails_model');
$this->load->model('tradie_model');

if($this->session->userdata('user_team_id') != 40)

$this->session->set_flashdata('error', 'You do not have permission to delete this job');
redirect($_SERVER['HTTP_REFERER']);
return;


$data = $this->jobs_model->with_tradie()->get($job_id);

$tradie_data = $this->tradie_model->get($data->tradie->tradie_id);

$cancelled_email = $this->jobs_model->build_job_cancelled_email($job_id, $tradie_data);
$this->send_emails_model->insert($cancelled_email);

if ($this->jobs_model->delete($job_id))
$this->session->set_flashdata('msg', 'Job has been successfully deleted');
else
$this->session->set_flashdata('error', 'Something went wrong, contact an administrator');


redirect($_SERVER['HTTP_REFERER']);




Thanks for the help!







share|improve this question





















  • Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate.
    – Sam Onela
    Jan 9 at 2:31












up vote
2
down vote

favorite









up vote
2
down vote

favorite











I'm working on a project and one of my controllers is getting quite large and hard to maintain or add to. I'm having trouble deciding what belongs in the controller and what I can move to the model. What can I change to improve this?



class Job_update extends MY_Controller


function __construct()

parent::__construct();
$this->load->helper('form');
$this->load->library('email');
$this->load->helper('html');
$this->load->model('jobs_model');

/**
* Reassigns a tradie to the work order sending appropriate emails out
* @param int $job_id
*/
public function reassign_tradie($job_id)

$this->load->model('send_emails_model');
$this->load->model('tradie_model');
$this->load->model('notes_model');
//get tradie ids via get or post
if ($this->input->get("tradie"))
$tradie_id = $this->input->get("tradie");
$prev_tradie = $this->input->get("prev_tradie");
else
$tradie_id = $this->input->post("tradie");
$prev_tradie = $this->input->post("prev_tradie");


if ($tradie_id == $prev_tradie)
$this->session->set_flashdata('error', "That tradesman is already assigned to this job");
redirect('job_update/job_id/'.$job_id);
return;


//retrieve data for both tradies
$data = $this->tradie_model->get($tradie_id); //new tradie
$prev_tradie_data = $this->tradie_model->get($prev_tradie);//prev tradie

//update the status to pending and set the new tradie
$this->jobs_model->update($job_id, array(
"status" => "Pending",
"tradie" => $tradie_id,
"job_tradie_response" => 0
));

//add a note to the job documenting the change
$job_note_message = "Tradesman has been changed from " . $prev_tradie_data->tradie_firstname .
" to ". $data->tradie_firstname . " Reason: " . $this->input->post("reason");
$this->notes_model->insert($job_id, $job_note_message);

//select the new job data
$job_data = $this->jobs_model->with_site_location()->with_tradie()->get($job_id);

//send the tradie an email of the work order
$tradie_email = $this->jobs_model->build_tradie_email($job_id, $job_data);
$this->send_emails_model->insert($tradie_email);

//if the prev tradie didn't decline the job we need to send a cancellation email
if ($job_data->status != "Declined")
$this->load->model('tradie_model');

$tradie_data = $this->tradie_model->get($prev_tradie);

$cancelled_email = $this->jobs_model->build_job_cancelled_email($job_id, $tradie_data);
$this->send_emails_model->insert($cancelled_email);


//redirect
$this->session->set_flashdata('msg', "Tradesman has been successfully changed");
redirect('job_update/job_id/'.$job_id);


/**
* Resends the work order to the assigned plumber also updates the status to pending
* @param int $job_id
*/
public function resend_work_order($job_id)

$this->load->model('notes_model');
$this->load->model('send_emails_model');
//updates status of job
$job_update = array("status" => "Pending", "job_tradie_response" => 0);
$this->jobs_model->update($job_update, $job_id);

//Sends email to tradesman
$job_data = $this->jobs_model->with_site_location()->with_tradie()->get($job_id);
$result = $this->jobs_model->build_tradie_email($job_id, $job_data);

$this->send_emails_model->insert($result);

//addes note to system talking about change
$data = array(
'note' => "Job has been resent to the plumber. Status changed to Pending",
'notes_job_id' => $job_id,
'datestamp' => date("Y-m-d H:i:s"),
'user_id' => $this->session->userdata('username')
);
$this->notes_model->insert($data);

$this->session->set_flashdata('msg', 'Resent Work Order to '. $job_data->tradie->tradie_firstname);
redirect('job_update/job_id/'.$job_id);


/**
* Inserts a new note from post data
* @param int $job_id
*/
public function job_add_note($job_id)

$this->_add_note($job_id, $this->input->post('job_notes'));

$this->session->set_flashdata('msg', 'Job Note Added Successfully');
redirect('job_update/job_id/'.$job_id);


/**
* Loads the photos page for a certain job
* @param int $job_id
*/
public function photos($job_id)

$data = array('job_id' => $job_id);
$this->load->view('job_photos_view', $data);



public function set_flash_for_bookin($job_id)

$this->session->set_flashdata('msg', 'Date/Time has been successfully set');
redirect('job_update/job_id/'.$job_id);


/**
* Saves the work order details (checkboxes, job_description)
* @param int $job_id
*/
public function submit_work_order($job_id)

$data = $this->update_checkboxes($job_id); //get all checkbox data
$this->jobs_model->update($data, $job_id); //save to db
$this->session->set_flashdata('msg', 'Work Order Updated Successfully');
redirect('job_update/job_id/'.$job_id);



/**
* Resends the customer confirmation email
* @param string (email) $c_email
* @param int $job_id
*/
public function resend_confirmation_email($c_email, $job_id)

$this->load->model('send_emails_model');
$this->job_update_model->update_send_email(array("email_sent" => 0), urldecode($c_email));

$this->session->set_flashdata('msg', 'Email Successfully Sent');
redirect('job_update/job_id/'.$job_id);


/**
* Sets the date for a checkbox
* @param string $post_data
* @param date $current_date_in_db
* @param bool (0,1) $is_checked
* @return date string format YYYY-MM-DD
*/
private function checkbox_helper($post_data, $current_date_in_db, $is_checked)
if($post_data == false)
return "0000-00-00";
else if($post_data != false && $is_checked === "0")
return date("Y-m-d");
else
return $current_date_in_db;


/**
* returns an array with checkbox data
* @param int $job_id
* @return array
*/
public function update_checkboxes($job_id)
$job_data = $this->jobs_model->get($job_id);

$current_date = date('Y-m-d');
$current_time = date('H:i:s');

//if 1 box is checked if 0 box is not checked
$data = array(
'box_customer_called' => ($this->input->post('checkbox1') == false ? '0' : '1'),
'box_customer_called_date' => $this->checkbox_helper($this->input->post('checkbox1'), $job_data->box_customer_called_date, $job_data->box_customer_called),
'box_photos_reviewed' => ($this->input->post('checkbox2') == false ? '0' : '1'),
'box_photos_received_date' => $this->checkbox_helper($this->input->post('checkbox2'), $job_data->box_photos_received_date, $job_data->box_photos_reviewed),
'box_photos_received' => ($this->input->post('checkbox6') == false ? '0' : '1'),
'box_photos_received_date' => $this->checkbox_helper($this->input->post('checkbox6'), $job_data->box_photos_received_date, $job_data->box_photos_received),
'box_completed' => ($this->input->post('checkbox7') == false ? '0' : '1'),
'box_completed_date' => $this->checkbox_helper($this->input->post('checkbox7'), $job_data->box_completed_date, $job_data->box_completed),
'box_completion_certificate' => ($this->input->post('checkbox3') == false ? '0' : '1'),
'box_completion_certificate_date' => $this->checkbox_helper($this->input->post('checkbox3'), $job_data->box_completion_certificate_date, $job_data->box_completion_certificate),
'box_invoiced' => ($this->input->post('checkbox4') == false ? '0' : '1'),
'box_invoiced_date' => $this->checkbox_helper($this->input->post('checkbox4'), $job_data->box_invoiced_date, $job_data->box_invoiced),
'box_warranty' => ($this->input->post('checkbox5') == false ? '0' : '1'),
'box_warranty_date' => $this->checkbox_helper($this->input->post('checkbox5'), $job_data->box_warranty_date, $job_data->box_warranty),
'job_description' => $this->input->post('job_description_new'),
);

//if the job was completed and is now unticked change the status
if($job_data->box_completed == 0 && $this->input->post('checkbox7') == "Completed")
$data['status'] = "Completed";

if($job_data->box_completed == 1 && $this->input->post('checkbox7') != "Completed")
$data['status'] = "Accepted";

return $data;


/**
* Inserts an email into the send que
* @param string $message
* @param string $email
* @param string $name
* @param string $email_subject
*/
public function email($message, $email, $name, $email_subject)

$this->load->model('send_emails_model');

$data = array('message' => $message,
'email' => $email,
'name' => $name,
'email_subject' => $email_subject);

$this->send_emails_model->insert($data);


/**
* Uploads a photo to the job directotry
* @param int $job_id
*/
public function upload_images($job_id)


$name_array = array();

if (!is_dir('assets/job_photos/'.add_prefix($job_id))) //create directory is it doesn't exist
mkdir('./assets/job_photos/'.add_prefix($job_id), 0777, true);
$dir_exist = false; // dir not exist


$count = count($_FILES['userfile']['size']);
foreach ($_FILES as $key => $value)
for ($s=0; $s<=$count-1; $s++)
$_FILES['userfile']['name']=$value['name'][$s];
$_FILES['userfile']['type'] = $value['type'][$s];
$_FILES['userfile']['tmp_name'] = $value['tmp_name'][$s];
$_FILES['userfile']['error'] = $value['error'][$s];
$_FILES['userfile']['size'] = $value['size'][$s];
$config['upload_path'] = './assets/job_photos/'.add_prefix($job_id).'/';
$config['allowed_types'] = 'gif

redirect(site_url('job_update/job_id/'.$job_id));


public function job_id($job_id)

$this->load->model('user_model');
$this->load->model('tradie_model');
$this->load->model('send_emails_model');
$this->load->model('notes_model');

//gather all data required for display
$job_data = $this->jobs_model->with_site_location()->with_tradie()->with_job_notes()->get($job_id);

$all_users['users'] = $this->user_model->select_all_users();
$tradies['tradies'] = $this->tradie_model->get_all();

$email = $this->send_emails_model->where('email', $job_data->site_location->tenant_email)->get();


//if job isn't found job data will be empty so we load a blank page and return
if (empty($job_data))
$this->headerlib->load_header();
return;


//change all 1's to checked for view
foreach ($job_data as $key => &$value)
//filter thourgh all table coloumns to find the boxes coloumns
if(strpos($key, 'box') !== false && strpos($key, 'date') === false)
if($value == 1)
$value = "checked";



$data = array_merge((array)$job_data, (array)$all_users, (array)$job_emails, (array)$tradies);

$this->headerlib->load_header();
$this->footerlib->load_footer();
$this->load->view('job_update_view', $data);


/**
* Deletes a job
* @param int $job_id job_id you wish to delete
*/
public function delete_job($job_id)

$this->load->model('send_emails_model');
$this->load->model('tradie_model');

if($this->session->userdata('user_team_id') != 40)

$this->session->set_flashdata('error', 'You do not have permission to delete this job');
redirect($_SERVER['HTTP_REFERER']);
return;


$data = $this->jobs_model->with_tradie()->get($job_id);

$tradie_data = $this->tradie_model->get($data->tradie->tradie_id);

$cancelled_email = $this->jobs_model->build_job_cancelled_email($job_id, $tradie_data);
$this->send_emails_model->insert($cancelled_email);

if ($this->jobs_model->delete($job_id))
$this->session->set_flashdata('msg', 'Job has been successfully deleted');
else
$this->session->set_flashdata('error', 'Something went wrong, contact an administrator');


redirect($_SERVER['HTTP_REFERER']);




Thanks for the help!







share|improve this question













I'm working on a project and one of my controllers is getting quite large and hard to maintain or add to. I'm having trouble deciding what belongs in the controller and what I can move to the model. What can I change to improve this?



class Job_update extends MY_Controller


function __construct()

parent::__construct();
$this->load->helper('form');
$this->load->library('email');
$this->load->helper('html');
$this->load->model('jobs_model');

/**
* Reassigns a tradie to the work order sending appropriate emails out
* @param int $job_id
*/
public function reassign_tradie($job_id)

$this->load->model('send_emails_model');
$this->load->model('tradie_model');
$this->load->model('notes_model');
//get tradie ids via get or post
if ($this->input->get("tradie"))
$tradie_id = $this->input->get("tradie");
$prev_tradie = $this->input->get("prev_tradie");
else
$tradie_id = $this->input->post("tradie");
$prev_tradie = $this->input->post("prev_tradie");


if ($tradie_id == $prev_tradie)
$this->session->set_flashdata('error', "That tradesman is already assigned to this job");
redirect('job_update/job_id/'.$job_id);
return;


//retrieve data for both tradies
$data = $this->tradie_model->get($tradie_id); //new tradie
$prev_tradie_data = $this->tradie_model->get($prev_tradie);//prev tradie

//update the status to pending and set the new tradie
$this->jobs_model->update($job_id, array(
"status" => "Pending",
"tradie" => $tradie_id,
"job_tradie_response" => 0
));

//add a note to the job documenting the change
$job_note_message = "Tradesman has been changed from " . $prev_tradie_data->tradie_firstname .
" to ". $data->tradie_firstname . " Reason: " . $this->input->post("reason");
$this->notes_model->insert($job_id, $job_note_message);

//select the new job data
$job_data = $this->jobs_model->with_site_location()->with_tradie()->get($job_id);

//send the tradie an email of the work order
$tradie_email = $this->jobs_model->build_tradie_email($job_id, $job_data);
$this->send_emails_model->insert($tradie_email);

//if the prev tradie didn't decline the job we need to send a cancellation email
if ($job_data->status != "Declined")
$this->load->model('tradie_model');

$tradie_data = $this->tradie_model->get($prev_tradie);

$cancelled_email = $this->jobs_model->build_job_cancelled_email($job_id, $tradie_data);
$this->send_emails_model->insert($cancelled_email);


//redirect
$this->session->set_flashdata('msg', "Tradesman has been successfully changed");
redirect('job_update/job_id/'.$job_id);


/**
* Resends the work order to the assigned plumber also updates the status to pending
* @param int $job_id
*/
public function resend_work_order($job_id)

$this->load->model('notes_model');
$this->load->model('send_emails_model');
//updates status of job
$job_update = array("status" => "Pending", "job_tradie_response" => 0);
$this->jobs_model->update($job_update, $job_id);

//Sends email to tradesman
$job_data = $this->jobs_model->with_site_location()->with_tradie()->get($job_id);
$result = $this->jobs_model->build_tradie_email($job_id, $job_data);

$this->send_emails_model->insert($result);

//addes note to system talking about change
$data = array(
'note' => "Job has been resent to the plumber. Status changed to Pending",
'notes_job_id' => $job_id,
'datestamp' => date("Y-m-d H:i:s"),
'user_id' => $this->session->userdata('username')
);
$this->notes_model->insert($data);

$this->session->set_flashdata('msg', 'Resent Work Order to '. $job_data->tradie->tradie_firstname);
redirect('job_update/job_id/'.$job_id);


/**
* Inserts a new note from post data
* @param int $job_id
*/
public function job_add_note($job_id)

$this->_add_note($job_id, $this->input->post('job_notes'));

$this->session->set_flashdata('msg', 'Job Note Added Successfully');
redirect('job_update/job_id/'.$job_id);


/**
* Loads the photos page for a certain job
* @param int $job_id
*/
public function photos($job_id)

$data = array('job_id' => $job_id);
$this->load->view('job_photos_view', $data);



public function set_flash_for_bookin($job_id)

$this->session->set_flashdata('msg', 'Date/Time has been successfully set');
redirect('job_update/job_id/'.$job_id);


/**
* Saves the work order details (checkboxes, job_description)
* @param int $job_id
*/
public function submit_work_order($job_id)

$data = $this->update_checkboxes($job_id); //get all checkbox data
$this->jobs_model->update($data, $job_id); //save to db
$this->session->set_flashdata('msg', 'Work Order Updated Successfully');
redirect('job_update/job_id/'.$job_id);



/**
* Resends the customer confirmation email
* @param string (email) $c_email
* @param int $job_id
*/
public function resend_confirmation_email($c_email, $job_id)

$this->load->model('send_emails_model');
$this->job_update_model->update_send_email(array("email_sent" => 0), urldecode($c_email));

$this->session->set_flashdata('msg', 'Email Successfully Sent');
redirect('job_update/job_id/'.$job_id);


/**
* Sets the date for a checkbox
* @param string $post_data
* @param date $current_date_in_db
* @param bool (0,1) $is_checked
* @return date string format YYYY-MM-DD
*/
private function checkbox_helper($post_data, $current_date_in_db, $is_checked)
if($post_data == false)
return "0000-00-00";
else if($post_data != false && $is_checked === "0")
return date("Y-m-d");
else
return $current_date_in_db;


/**
* returns an array with checkbox data
* @param int $job_id
* @return array
*/
public function update_checkboxes($job_id)
$job_data = $this->jobs_model->get($job_id);

$current_date = date('Y-m-d');
$current_time = date('H:i:s');

//if 1 box is checked if 0 box is not checked
$data = array(
'box_customer_called' => ($this->input->post('checkbox1') == false ? '0' : '1'),
'box_customer_called_date' => $this->checkbox_helper($this->input->post('checkbox1'), $job_data->box_customer_called_date, $job_data->box_customer_called),
'box_photos_reviewed' => ($this->input->post('checkbox2') == false ? '0' : '1'),
'box_photos_received_date' => $this->checkbox_helper($this->input->post('checkbox2'), $job_data->box_photos_received_date, $job_data->box_photos_reviewed),
'box_photos_received' => ($this->input->post('checkbox6') == false ? '0' : '1'),
'box_photos_received_date' => $this->checkbox_helper($this->input->post('checkbox6'), $job_data->box_photos_received_date, $job_data->box_photos_received),
'box_completed' => ($this->input->post('checkbox7') == false ? '0' : '1'),
'box_completed_date' => $this->checkbox_helper($this->input->post('checkbox7'), $job_data->box_completed_date, $job_data->box_completed),
'box_completion_certificate' => ($this->input->post('checkbox3') == false ? '0' : '1'),
'box_completion_certificate_date' => $this->checkbox_helper($this->input->post('checkbox3'), $job_data->box_completion_certificate_date, $job_data->box_completion_certificate),
'box_invoiced' => ($this->input->post('checkbox4') == false ? '0' : '1'),
'box_invoiced_date' => $this->checkbox_helper($this->input->post('checkbox4'), $job_data->box_invoiced_date, $job_data->box_invoiced),
'box_warranty' => ($this->input->post('checkbox5') == false ? '0' : '1'),
'box_warranty_date' => $this->checkbox_helper($this->input->post('checkbox5'), $job_data->box_warranty_date, $job_data->box_warranty),
'job_description' => $this->input->post('job_description_new'),
);

//if the job was completed and is now unticked change the status
if($job_data->box_completed == 0 && $this->input->post('checkbox7') == "Completed")
$data['status'] = "Completed";

if($job_data->box_completed == 1 && $this->input->post('checkbox7') != "Completed")
$data['status'] = "Accepted";

return $data;


/**
* Inserts an email into the send que
* @param string $message
* @param string $email
* @param string $name
* @param string $email_subject
*/
public function email($message, $email, $name, $email_subject)

$this->load->model('send_emails_model');

$data = array('message' => $message,
'email' => $email,
'name' => $name,
'email_subject' => $email_subject);

$this->send_emails_model->insert($data);


/**
* Uploads a photo to the job directotry
* @param int $job_id
*/
public function upload_images($job_id)


$name_array = array();

if (!is_dir('assets/job_photos/'.add_prefix($job_id))) //create directory is it doesn't exist
mkdir('./assets/job_photos/'.add_prefix($job_id), 0777, true);
$dir_exist = false; // dir not exist


$count = count($_FILES['userfile']['size']);
foreach ($_FILES as $key => $value)
for ($s=0; $s<=$count-1; $s++)
$_FILES['userfile']['name']=$value['name'][$s];
$_FILES['userfile']['type'] = $value['type'][$s];
$_FILES['userfile']['tmp_name'] = $value['tmp_name'][$s];
$_FILES['userfile']['error'] = $value['error'][$s];
$_FILES['userfile']['size'] = $value['size'][$s];
$config['upload_path'] = './assets/job_photos/'.add_prefix($job_id).'/';
$config['allowed_types'] = 'gif

redirect(site_url('job_update/job_id/'.$job_id));


public function job_id($job_id)

$this->load->model('user_model');
$this->load->model('tradie_model');
$this->load->model('send_emails_model');
$this->load->model('notes_model');

//gather all data required for display
$job_data = $this->jobs_model->with_site_location()->with_tradie()->with_job_notes()->get($job_id);

$all_users['users'] = $this->user_model->select_all_users();
$tradies['tradies'] = $this->tradie_model->get_all();

$email = $this->send_emails_model->where('email', $job_data->site_location->tenant_email)->get();


//if job isn't found job data will be empty so we load a blank page and return
if (empty($job_data))
$this->headerlib->load_header();
return;


//change all 1's to checked for view
foreach ($job_data as $key => &$value)
//filter thourgh all table coloumns to find the boxes coloumns
if(strpos($key, 'box') !== false && strpos($key, 'date') === false)
if($value == 1)
$value = "checked";



$data = array_merge((array)$job_data, (array)$all_users, (array)$job_emails, (array)$tradies);

$this->headerlib->load_header();
$this->footerlib->load_footer();
$this->load->view('job_update_view', $data);


/**
* Deletes a job
* @param int $job_id job_id you wish to delete
*/
public function delete_job($job_id)

$this->load->model('send_emails_model');
$this->load->model('tradie_model');

if($this->session->userdata('user_team_id') != 40)

$this->session->set_flashdata('error', 'You do not have permission to delete this job');
redirect($_SERVER['HTTP_REFERER']);
return;


$data = $this->jobs_model->with_tradie()->get($job_id);

$tradie_data = $this->tradie_model->get($data->tradie->tradie_id);

$cancelled_email = $this->jobs_model->build_job_cancelled_email($job_id, $tradie_data);
$this->send_emails_model->insert($cancelled_email);

if ($this->jobs_model->delete($job_id))
$this->session->set_flashdata('msg', 'Job has been successfully deleted');
else
$this->session->set_flashdata('error', 'Something went wrong, contact an administrator');


redirect($_SERVER['HTTP_REFERER']);




Thanks for the help!









share|improve this question












share|improve this question




share|improve this question








edited Jan 9 at 2:32









Sam Onela

5,88461545




5,88461545









asked Jan 9 at 1:32









rtzcoder

132




132











  • Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate.
    – Sam Onela
    Jan 9 at 2:31
















  • Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate.
    – Sam Onela
    Jan 9 at 2:31















Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate.
– Sam Onela
Jan 9 at 2:31




Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate.
– Sam Onela
Jan 9 at 2:31










1 Answer
1






active

oldest

votes

















up vote
1
down vote



accepted










  • Models are for data models, you shold not be using them to do things like sending emails, you should be using libraries for that kind of task instead, and have the library insert a record into the model if required.

  • Your $data array in update_checkboxes() should be generated in a model, since you're creating a data model. I would accomplish this by adding a method to the jobs model and passing it the user input, then have it return that array/model, eg $data = $this->jobs_model->get_data_array($this->input);

  • You shouldn't be assigning things to the $_FILES superglobal (or any superglobals) you should pass the required $values array to the library directly. Also, your $config variables don't change in the loop so why re-assign them on every iteration.

  • You could use array_map to change all the 1s to checked, although that's probably the type of thing that should be done in a model.





share|improve this answer





















  • Thanks this will help a lot! Do you think the long functions like reassign_tradie($job_id) should be broken up into smaller functions?
    – rtzcoder
    Jan 9 at 23:17










  • @rtzcoder ideally all functions would be short but by the same token, you shouldn't make your functions overly short just because some convention says you should. controllers are usually the exception. some of the CI controllers we have at my job are extremely long and there's no obvious semantic way to shorten them. I think your method is fine and I can't see an obvious way to shorten it.
    – Occam's Razor
    Jan 10 at 0:18










Your Answer




StackExchange.ifUsing("editor", function ()
return StackExchange.using("mathjaxEditing", function ()
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
);
);
, "mathjax-editing");

StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");

StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);

else
createEditor();

);

function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: false,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);



);








 

draft saved


draft discarded


















StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f184629%2fcodeigniter-controller-for-work-orders%23new-answer', 'question_page');

);

Post as a guest






























1 Answer
1






active

oldest

votes








1 Answer
1






active

oldest

votes









active

oldest

votes






active

oldest

votes








up vote
1
down vote



accepted










  • Models are for data models, you shold not be using them to do things like sending emails, you should be using libraries for that kind of task instead, and have the library insert a record into the model if required.

  • Your $data array in update_checkboxes() should be generated in a model, since you're creating a data model. I would accomplish this by adding a method to the jobs model and passing it the user input, then have it return that array/model, eg $data = $this->jobs_model->get_data_array($this->input);

  • You shouldn't be assigning things to the $_FILES superglobal (or any superglobals) you should pass the required $values array to the library directly. Also, your $config variables don't change in the loop so why re-assign them on every iteration.

  • You could use array_map to change all the 1s to checked, although that's probably the type of thing that should be done in a model.





share|improve this answer





















  • Thanks this will help a lot! Do you think the long functions like reassign_tradie($job_id) should be broken up into smaller functions?
    – rtzcoder
    Jan 9 at 23:17










  • @rtzcoder ideally all functions would be short but by the same token, you shouldn't make your functions overly short just because some convention says you should. controllers are usually the exception. some of the CI controllers we have at my job are extremely long and there's no obvious semantic way to shorten them. I think your method is fine and I can't see an obvious way to shorten it.
    – Occam's Razor
    Jan 10 at 0:18














up vote
1
down vote



accepted










  • Models are for data models, you shold not be using them to do things like sending emails, you should be using libraries for that kind of task instead, and have the library insert a record into the model if required.

  • Your $data array in update_checkboxes() should be generated in a model, since you're creating a data model. I would accomplish this by adding a method to the jobs model and passing it the user input, then have it return that array/model, eg $data = $this->jobs_model->get_data_array($this->input);

  • You shouldn't be assigning things to the $_FILES superglobal (or any superglobals) you should pass the required $values array to the library directly. Also, your $config variables don't change in the loop so why re-assign them on every iteration.

  • You could use array_map to change all the 1s to checked, although that's probably the type of thing that should be done in a model.





share|improve this answer





















  • Thanks this will help a lot! Do you think the long functions like reassign_tradie($job_id) should be broken up into smaller functions?
    – rtzcoder
    Jan 9 at 23:17










  • @rtzcoder ideally all functions would be short but by the same token, you shouldn't make your functions overly short just because some convention says you should. controllers are usually the exception. some of the CI controllers we have at my job are extremely long and there's no obvious semantic way to shorten them. I think your method is fine and I can't see an obvious way to shorten it.
    – Occam's Razor
    Jan 10 at 0:18












up vote
1
down vote



accepted







up vote
1
down vote



accepted






  • Models are for data models, you shold not be using them to do things like sending emails, you should be using libraries for that kind of task instead, and have the library insert a record into the model if required.

  • Your $data array in update_checkboxes() should be generated in a model, since you're creating a data model. I would accomplish this by adding a method to the jobs model and passing it the user input, then have it return that array/model, eg $data = $this->jobs_model->get_data_array($this->input);

  • You shouldn't be assigning things to the $_FILES superglobal (or any superglobals) you should pass the required $values array to the library directly. Also, your $config variables don't change in the loop so why re-assign them on every iteration.

  • You could use array_map to change all the 1s to checked, although that's probably the type of thing that should be done in a model.





share|improve this answer













  • Models are for data models, you shold not be using them to do things like sending emails, you should be using libraries for that kind of task instead, and have the library insert a record into the model if required.

  • Your $data array in update_checkboxes() should be generated in a model, since you're creating a data model. I would accomplish this by adding a method to the jobs model and passing it the user input, then have it return that array/model, eg $data = $this->jobs_model->get_data_array($this->input);

  • You shouldn't be assigning things to the $_FILES superglobal (or any superglobals) you should pass the required $values array to the library directly. Also, your $config variables don't change in the loop so why re-assign them on every iteration.

  • You could use array_map to change all the 1s to checked, although that's probably the type of thing that should be done in a model.






share|improve this answer













share|improve this answer



share|improve this answer











answered Jan 9 at 2:28









Occam's Razor

1,982513




1,982513











  • Thanks this will help a lot! Do you think the long functions like reassign_tradie($job_id) should be broken up into smaller functions?
    – rtzcoder
    Jan 9 at 23:17










  • @rtzcoder ideally all functions would be short but by the same token, you shouldn't make your functions overly short just because some convention says you should. controllers are usually the exception. some of the CI controllers we have at my job are extremely long and there's no obvious semantic way to shorten them. I think your method is fine and I can't see an obvious way to shorten it.
    – Occam's Razor
    Jan 10 at 0:18
















  • Thanks this will help a lot! Do you think the long functions like reassign_tradie($job_id) should be broken up into smaller functions?
    – rtzcoder
    Jan 9 at 23:17










  • @rtzcoder ideally all functions would be short but by the same token, you shouldn't make your functions overly short just because some convention says you should. controllers are usually the exception. some of the CI controllers we have at my job are extremely long and there's no obvious semantic way to shorten them. I think your method is fine and I can't see an obvious way to shorten it.
    – Occam's Razor
    Jan 10 at 0:18















Thanks this will help a lot! Do you think the long functions like reassign_tradie($job_id) should be broken up into smaller functions?
– rtzcoder
Jan 9 at 23:17




Thanks this will help a lot! Do you think the long functions like reassign_tradie($job_id) should be broken up into smaller functions?
– rtzcoder
Jan 9 at 23:17












@rtzcoder ideally all functions would be short but by the same token, you shouldn't make your functions overly short just because some convention says you should. controllers are usually the exception. some of the CI controllers we have at my job are extremely long and there's no obvious semantic way to shorten them. I think your method is fine and I can't see an obvious way to shorten it.
– Occam's Razor
Jan 10 at 0:18




@rtzcoder ideally all functions would be short but by the same token, you shouldn't make your functions overly short just because some convention says you should. controllers are usually the exception. some of the CI controllers we have at my job are extremely long and there's no obvious semantic way to shorten them. I think your method is fine and I can't see an obvious way to shorten it.
– Occam's Razor
Jan 10 at 0:18












 

draft saved


draft discarded


























 


draft saved


draft discarded














StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f184629%2fcodeigniter-controller-for-work-orders%23new-answer', 'question_page');

);

Post as a guest













































































Popular posts from this blog

Python Lists

Aion

JavaScript Array Iteration Methods