Give me feedback

First try after watching "Your code s*cks" - a codeigniter controller's code

I've rewritten the controller of my newest project's control page, hope it s*cks less now. :)

admin.php
<?php if ( ! defined('BASEPATH')) exit('No direct script access allowed'); class Admin extends CI_Controller { public function __construct(){ parent::__construct(); $this->load->library('ion_auth'); if (!$this->ion_auth->logged_in() || !$this->ion_auth->is_admin()) redirect('auth/login', 'refresh'); $this->load->model('db_functions_model'); } /* Loads page with header, nav and footer */ private function _render_page($pagename, $data=Array()){ $this->load->view('admin/includes/header', $data, FALSE); $this->load->view('admin/includes/nav', $data, FALSE); $this->load->view('admin/'.$pagename, $data, FALSE); $this->load->view('admin/includes/footer', $data, FALSE); } /* Loads showtable view and passes table's content */ public function showTable($table){ switch($table){ case 'plans': $data['title'] = 'Képzések'; break; case 'plansemesters': $data['title'] = 'Mintatantervi félévek'; break; case 'subjects': $data['title'] = 'Tárgyak'; break; default: $data['title'] = '(Ismeretlen)'; break; } $data['table'] = $table; $data['results'] = $this->db->get($table)->result(); $this->_render_page('showtable', $data); } /* Updates row of table (codes are used for identification) */ public function updateRow($table, $row_code){ $this->db_functions_model->update('code', $row_code, Array('name' => $this->input->post('name')), $table); redirect('admin/showTable/'.$table); } /* Inserts new row to table */ public function insertRow($table){ $this->db->insert($table, Array('code' => $this->input->post('code'), 'name' => $this->input->post('name'))); redirect('admin/showTable/'.$table); } /* Deletes row from table */ public function deleteRow($table, $row_code){ $this->db_functions_model->delete('code', $row_code, $table); if($table == "plans") //special case: delete connections to subjects too $this->db_functions_model->delete('plan_code', $row_code, 'plans_has_subjects'); redirect('admin/showTable/'.$table); } /* Loads events view and passes possible options for dropdowns */ public function showEvents($plan_code = FALSE, $semester = FALSE){ if(!$plan_code) $plan_code = $this->input->post('plan_code', FALSE); if(!$plan_code) $plan_code = $this->db->order_by('code', 'asc')->get('plans')->row()->code; //first one is default if(!$semester) $semester = $this->input->post('semester', FALSE); if(!$semester) $semester = $this->db->order_by('semester', 'asc')->get('events')->row()->semester; //first one is default $data['plan_code'] = $plan_code; $data['semester'] = $semester; $data['results'] = $this->db->select('events.*, subjects.name subject_name') ->where('plan_code', $plan_code) ->where('semester', str_replace('_','/',$semester)) ->join('plans_has_subjects', 'plans_has_subjects.subject_code = events.subject_code', 'left') ->join('subjects', 'events.subject_code = subjects.code', 'left') ->get('events')->result(); $data['plans'] = $this->db->order_by('name', 'asc')->get('plans')->result(); $data['semesters'] = $this->db->order_by('semester', 'asc')->group_by('semester')->get('events')->result(); $this->_render_page('events', $data); } /* Deletes event */ public function deleteEvent($plan_code, $semester, $event_id){ $this->db_functions_model->delete('id', $event_id, 'events'); redirect('admin/showEvents/'.$plan_code.'/'.$semester); } /* Loads plans_subjects view and passes possible options for dropdowns */ public function showPlansSubjects($plan_code = FALSE){ if( ! $plan_code) $plan_code = $this->input->post('plan_code', FALSE); if( ! $plan_code) $plan_code = $this->db->order_by('code', 'asc')->get('plans')->row()->code; //first one is default $this->db->select('plans.name plan_name, plan_code, plansemesters.name plansemester_name, plansemesters.code plansemester_code, subjects.*') ->join('subjects', 'subject_code = subjects.code', 'left') ->join('plansemesters', 'plansemester_code = plansemesters.code', 'left') ->join('plans', 'plan_code = plans.code', 'left') ->where('plan_code', $plan_code) ->order_by('plansemester_name', 'asc'); $data['results'] = $this->db->get('plans_has_subjects')->result(); $data['plan_code'] = $plan_code; $data['plans'] = $this->db->order_by('name', 'asc')->get('plans')->result(); $data['plansemesters'] = $this->db->order_by('name', 'asc')->get('plansemesters')->result(); $data['allsubjects'] = $this->db->order_by('name', 'asc')->get('subjects')->result(); $this->_render_page('plans_subjects', $data); } /* Deletes connection between a subject and a plan */ public function disconnectSubject($plan_code, $plansemester_code, $subject_code){ $this->db->where('subject_code', $subject_code) ->where('plansemester_code', $plansemester_code); $this->db_functions_model->delete('plan_code', $plan_code, 'plans_has_subjects'); redirect('admin/showPlansSubjects/'.$plan_code); } /* Creates a connection between a subject and a plan */ public function connectSubject($plan_code){ $insertdata = Array( 'plan_code' => $plan_code, 'plansemester_code' => $this->input->post('plansemester_code'), 'subject_code' => $this->input->post('subject_code') ); $this->db_functions_model->insert($insertdata, 'plans_has_subjects'); redirect('admin/showPlansSubjects/'.$plan_code); } /* Loads import view and passes possible option for dropdown */ public function import(){ $data['plans'] = $this->db->order_by('name', 'asc')->get('plans')->result(); $this->_render_page('import', $data); } /* Import subjects from selected CSV file */ public function importSubjects(){ $this->load->model('csv_import_model'); $data['log'] = $this->csv_import_model->_import_subjects(); redirect('admin/import'); } /* Imports events from selected CSV file */ public function importEvents(){ $this->load->model('csv_import_model'); $data['log'] = $this->csv_import_model->_import_events(); redirect('admin/import'); } }

Reactions 1 reactions

rdohms Rafael Dohms commented on this code
56
The lack of curly brackets {} is a bit annoying to me, makes it less readable and error prone in my opinion. I would consider maybe abstracting some of the DB stuff like the SELECT into a repository where you just pass in the parameters and it builds the join.

Comment on this code:

Please Login or Register to leave a comment.