I am currently working in CodeIgniter. I’m building an Admin for a website and basically doing CRUD over and over. I am attempting to now just build a base class that can take care of most of the behaviors I need and then just override whatever else.
Here is what I attempting for a base class. I’m not quite sure where to go with this at all.
<?php
class Base Extends Controller {
protected $_model;
public function __construct() {
parent::__construct();
}
public function add($id = NULL) {
if ($this->input->post('add')) {
if ($this->$this->_model->add($data)) {
}
}
}
public function edit() {
}
public function delete() {
}
public function render($page) {
$this->load->view('template/header');
$this->load->view($page);
$this->load->view('template/footer');
}
private function is_set($data) {
return (isset($data) && !empty($data)) ? '1' : '0';
}
}
?>
Here is an example of what I normally do.
<?php
class Class_Pages extends Controller {
public function __construct() {
parent::Controller();
$this->load->Model('Class_Pages_model', 'Pages');
}
public function add($section_id, $class_id) {
$data['section_id'] = $section_id;
$data['class_id'] = $class_id;
if ($this->input->post('add_page')) {
$data = array(
'id' => 'NULL',
'section_id' => $section_id,
'title' => $this->input->post('title'),
'slug' => $this->input->post('slug'),
'content' => $this->input->post('content'),
'rank' => $this->input->post('rank')
);
if ($this->Pages->add_page($data)) {
$this->session->set_flashdata('message', '<div class="greenBox" id="message"> '
. '<strong>Sucessfully added Page:</strong> ' . $this->input->post('title') . '</div>');
} else {
$this->session->set_flashdata('message', '<div class="redBox" id="message"> Oops we were unable to add the Page. Please try again ! </div>');
}
redirect('/classes/manage/' . $class_id . '/' . $section_id);
}
$this->render('add', $data);
}
public function edit($page_id, $class_id) {
$data['page_id'] = $page_id;
$data['page'] = $this->Pages->get_page($page_id);
$section_id = $data['page']->section_id;
if ($this->input->post('edit_page')) {
$data = array(
'section_id' => $section_id,
'title' => $this->input->post('title'),
'slug' => $this->input->post('slug'),
'content' => $this->input->post('content'),
'rank' => $this->input->post('rank')
);
if ($this->Pages->edit_page($page_id, $data)) {
$this->session->set_flashdata('message', '<div class="greenBox" id="message"> '
. '<strong>Sucessfully edited Page:</strong> ' . $this->input->post('title') . '</div>');
} else {
$this->session->set_flashdata('message', '<div class="redBox" id="message"> Oops we were unable to edit the Page. Please try again ! </div>');
}
redirect('/classes/manage/' . $class_id . '/' . $section_id);
}
$this->render('edit', $data);
}
private function render($page, $data = NULL) {
$this->load->view('template/header');
$this->load->view('pages/' . $page, $data);
$this->load->view('template/footer');
}
}
?>
Any suggestions to make this code more DRY, so I’m not repeating the basic same functionality over and over would be awesome !
I usually have a single ‘save’ function that both the ‘edit’ and ‘create’ functions submit to. Additionally, both the ‘edit’ and ‘create’ functions display the same form - the only difference is the edit form will have an extra value specified for the record id that the ‘save’ function will pick up to load an existing record instead of inserting a new one.
Yeah, essentially I guess a function of ‘save’ would be more of the route to go like you said. I’m getting to the point where I am feeling that coding based of what CI teaches you isn’t the best of practice, etc.
It definitely isn’t the ultimate best of practices, but it works nevertheless. CI isn’t a hallmark of the most flexible or decent design, but it does help you to get the job done. There’s a lot you can do yourself to improve your code, before blaming CI on teaching you best practices. A good framework can definitely point in the right direction of design, but you won’t ever get there by mindlessly picking up stuff other people wrote and not think about it an awful lot. This is no offense; I’ve started approximately the same way, and I like to think I’m the better for it, in the end.
Now, for your question. Usually when I want to make code more DRY, I tend to look at the places that seem to repeat code, or repeat code with slight variations. You seem to have a few of those in your code. If I were to write a DRY version of your “base” controller, this is what I’d come up with.
<?php
class Class_Pages extends Controller
{
public function __construct( )
{
parent::__construct( );
$this->load->Model('Class_Pages_model', 'Pages');
}
public function add( $section_id, $class_id )
{
$data = array(
'section_id' => $section_id,
'class_id' => $class_id
);
if( $this->input->post( 'add_page' ) ) {
$data += $this->build( $section_id, null );
if( $this->Pages->add_page( $data ) ) {
$this->succes( sprintf( 'Succesfully added page: %s', $this->input->post( 'title' ) ) );
$this->redirect( '/classes/manage' );
}
$this->error( 'Oops we were unable to add the page. Please try again!' );
$this->redirect( '/classes/manage' );
}
$this->render( 'add', $data );
}
public function edit( $page_id, $class_id )
{
$data = array(
'page_id' => $page_id,
'page' => $this->Pages->get_page( $page_id )
);
$section_id = $data['page']->section_id;
if( $this->input->post( 'edit_page' ) ) {
$data += $this->build( $section_id );
if( $this->Pages->edit_page( $page_id, $data ) {
$this->success( sprintf( 'Succesfully edited Page: %s', $this->input->post( 'title' ) ) );
}
else {
$this->error( 'Oops we were unable to edit the page. Please try again.' );
}
$this->redirect( sprintf( '/classes/manage/%d/%d', $class_id, $section_id ) );
}
}
/**
* This method has an awful name. "build" is way to generic. Nevertheless, I'd actually
* build a model object here (say, page), and call this method getPage( $pageid ).
*/
protected function build( $section_id )
{
if( false !== $id ) {
$return = array( 'id' => $id );
}
return $return + array(
'section_id' => $section_id,
'title' => $this->input->post('title'),
'slug' => $this->input->post('slug'),
'content' => $this->input->post('content'),
'rank' => $this->input->post('rank')
);
}
/**
* I'd probably split these up into their own "base controller" if you use them more often.
* If you use it in different projects, I'd split it up in it's own helper.
*/
protected function redirect( $location )
{
redirect( $location );
exit;
}
protected function render( $page, $data = null ) {
$this->load->view( 'template/header' );
$this->load->view( 'pages/' . $page, $data );
$this->load->view( 'template/footer' );
}
protected function success( $message )
{
return $this->flash(
sprintf( '<div class="greenBox" id="message">%s</div>', $message )
);
}
protected function error( $message )
{
return $this->flash(
sprintf( '<div class="redBox" id="message">%s</div>', $message )
);
}
protected function flash( $message )
{
return $this->session->set_flashdata( 'message', $message );
}
}
I do have to make a few notes though, aside from the ones that I’ve written in the docblocks. First off all; if more than one controller is “burdened” with the control over the concept of “Pages”, you should soon see that you’re not only duplicating stuff in your controller, but also across controllers. Mind you that this is exactly the reason that the “Model” part of MVC has been invented; a Controller should do little more than talk to the model layer and choosing a view. It shouldn’t worry about trivial stuff such as layouts. It shouldn’t care that an error has to be displayed in a red box.
You have neglected the model, and the responsibilities that belong to the view. I know that when you start those concepts may sound very abstract, and there’s a good reason: they are. You have to gain experience, and fall flat on your face a few times before you actually get the hang of it. I blame the internet for it; there are legions of articles where the model layer is little more than a place to stuff your queries in, and I do believe those articles are blatently wrong. In my applications, the model actually makes up the better part of the lines of code, not the controller. Although, the view is quite heavy too.
One article I know of, written by Pádraic Brady, actually has it right and it triggered a very small chain reaction in the professional PHP world. I think this is an article which may shed a lot of light on the subject of Model View Controller, but if you’re not that into it, it might go over your head. I suggest you try it nevertheless. Stop making controllers do things, and your code will become a lot more DRY instantly. I’m not saying I have the answer to life, the universe and everything, but the way I work, definitely works for me. The controller doesn’t have much repetition in it, and if it does, I usually just extract the commonalities.
With decent knowledge of the model layer, and how to write it, the controller actually does nothing but determine whether to redirect, or display a page. If you DRY that off as well, I end up with a controller which is 54 lines, easy to read, and which essentially does nothing but calling the model and rendering a view. This probably isn’t the best practice that CI promotes, and it does require you to write code in other places.
For starters, I’ve made a Page a separate object, something I’d do in your case anyway. I’ve extracted the “construction” of said Page objects to a PageRepository, which coincidentally also knows where to locate the Pages (presumably in a database). Also, the PageRepository will be injected into the constructor of the PageController, which is impossible to do with CI as far as I know. Also, I’ve made a clear distinction between request types. HTTP GET requests only render stuff, HTTP POST request will end up in a redirecting response or a rendered view.
The view itself is responsible for getting the page, and seeing if it has had errors upon saving it. As such, the view will decide whether or not to display the errors and in what format it would like to do so. The view that gets called upon entering /pages/ will decide whether or not there is a flash message and if so, it will display it. All validation, filtering and escaping of the values is done in the model. All escaping of output and other output related things will be done in the view.
<?php
class PageController extends Controller
{
protected $pages;
public function __construct( PagesRepository $pages )
{
$this->pages = $pages;
}
public function renderAdd( )
{
return $this->render( '/pages/add' );
}
public function performAdd( )
{
$result = $this->save(
$this->build( $this->request->body( ) ),
'The page has been succesfully added.',
'/pages/'
);
return ( false !== $result ) ? $result : $this->renderAdd( );
}
public function renderEdit( )
{
return $this->render( '/pages/edit' );
}
public function performEdit( )
{
$result = $this->save(
$this->build( $this->request->body( ), $this->request->query( 'pageid' ) ),
'The page has been succesfully updated.',
'/pages/'
);
return ( false !== $result ) ? $result : $this->renderEdit( );
}
protected function build( Array $values, $id = null )
{
return $this->pages->build( $values, $id );
}
protected function save( Page $page, $flashmessage, $location )
{
if( $page->save( $this->pages ) ) {
$this->flash( $flashmessage );
return $this->redirect( $location );
}
return false;
}
}
?>
You declare a private function, yet you don’t use it. That makes it dead code. Besides that, it won’t work. isset is a construct - you can’t delegate it to a function like that; You would still get an error (warning) on an undefined variable.
public function __construct() {
parent::Controller();
$this->load->Model('Class_Pages_model', 'Pages');
}
This opens for xss injection. Not that critical, since it’s a POST source, but still. Escape user input with htmlspecialchars when embedding in html.
I don’t know what the standard for CI is, but I generally dislike the public keyword in front of functions, since it’s implicit. ymmv.
In general: I would suggest that you take two or three concrete working examples (as your second codeblock), then create an empty baseclass and make them extend from it. Then gradually move duplicated code from descendants to parent, and test that they still work between each change. It’s a much better workflow, than trying to construct a library separately, as it seems you’re doing. Maybe even write some automated tests to assert validity between changes.
I would add that $this->$this depends both on Controller::_toString, which you shouldn’t depend on for anything (i.e. what if they choose to change what it returns?). Further, the _model variable is an indication of a variable that should be private but doesn’t have a type specifier to make it so (in the case of PHP4 compatibility), and thus should not be used. $this->$this->_model is a maintenance nightmare.
This is redundant, if you want to give certain fields default values and extract specific array keys from something then use array_key_intersect() with an array full of defaults and the array to extract values from. I assume that you have access to the array, which might not be true. On second thought, array_key_intersect would result in similar code. Nevertheless, pulling such a technique out into its own helper function is nifty.
One article I know of, written by Pádraic Brady, actually has it right and it triggered a very small chain reaction in the professional PHP world. I think this is an article which may shed a lot of light on the subject of Model View Controller, but if you’re not that into it, it might go over your head. I suggest you try it nevertheless. Stop making controllers do things, and your code will become a lot more DRY instantly. I’m not saying I have the answer to life, the universe and everything, but the way I work, definitely works for me. The controller doesn’t have much repetition in it, and if it does, I usually just extract the commonalities.
I just read this. It definately sounds right, but I have a very hard time seing how to put this into practice, and he doesn’t supply any code examples.
For example a basic (heavily simplified) controller may look like:
class Controller {
public function list() {
//some method of retrieving an array of model objects
$models = $this->Model->fetchAll();
$view = new View('whatever.tpl.php);
$view->assign('Models', $models);
return $view->render();
}
public function add() {
$model = new Model();
//using POST for simplicity, the controller would obviously be passed request data in a different way.
foreach ($_POST as $key => $value) {
$model->$key = $value;
}
$this->dataMapper->save($model);
return $this->list();
}
public function remove($id) {
$this->dataMapper->remove($id, 'Model');
return $this->list();
}
}
In this example, the data access is done by the dataMapper, is he saying the dataMapper should be accessible within the model??? that sounds really wrong to me and you may as well use activerecord?
Can anyone supply an example with a “thin” controller? I guess my problem is I can’t see what should go in the controller and what should go in the model.
Can anyone supply an example with a “thin” controller? I guess my problem is I can’t see what should go in the controller and what should go in the model
Redirection and forwarding…application logic goes in the controller…everything else goes in the model.
class Controller{
function testAction()
{
$request = $this->getRequest();
$response = $this->getReponse();
$view = new View();
$view->fname = $request->getParam('fname', 'Alex Barylski');
// Very simple view rendering probably want two step view or composite
$response->setContent($view->render());
}
function testAction2()
{
$request = $this->getRequest();
$response = $this->getResponse();
$fname = $request->getParam('fname', '');
$model = new Model();
if(($result = $model->createPerson($fname) !== false){
$response->doRedirect('listings.php');
}
// Emulate a postback
$this->testAction(); // Crude forward - use built-in mechanism
}
}
Very little logic should ever finds it’s way into the controllers in fact I rarely have any as I don’t explicitly test model return values.
My models then perform:
Validation
Data access/manipulation
Locale formatting
The confusing part about ‘model’ is that it’s definition is so vague an yet its this layer that encompasses everything. I think domain model would be a better description of the model from the perspective of MVC, however a domain model can then be decomposed into several distinct mini-models, such as entity models, etc.
This is why so many people assume that any layer that simply interacts with the database becomes the model, which is correct, just incomplete.
It looks like it needs a lot of binding logic between the controller and the view:
$view = new View();
$view->fname = $request->getParam('fname', 'Alex Barylski');
Shouldn’t it be something more like:
$view->assign('request', $request);
This means the controller needs zero knowledge about what the request contains.
However, I’m still slightly confused about what a simple controller should look like according to the article posted, because unfortunately the article contains no example code.