Thread: More on MVC

    #1
  1. A Change of Season
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Mar 2004
    Location
    Next Door
    Posts
    2,650
    Rep Power
    171

    More on MVC


    Hello; I have 3 questions I appreciate if anyone helps.
    1 - Is there any design mistakes I am making here? Also little things like "where is the best place to use htmlentities?" Model view or controller? Im not sure.
    2 - What is the right way of making models? For example is it the right thing to have every member related method in one model? Or is it right to have one separate model per method?
    3 - Do you see any obvious security holes?
    This is one page and this is the other in case you need to see what it looks like.
    Thank you
    Model
    PHP Code:
    <?php if ( ! defined('BASEPATH')) exit('No direct script access allowed');
    class 
    Places_model extends CI_Model
        
    {
            public 
    $place_results;
            public 
    $ad_details;
            function 
    load_places()
                {
                    
    $query " SELECT ad_have.id AS AID,
           title,
           comments,
           date_added,
           suburb,
           city,
           city.name  AS cn,
           suburb
    FROM   ad_have
           INNER JOIN city
                   ON city.id = ad_have.city
           INNER JOIN members
                   ON members.id = ad_have.member_id
    ORDER  BY date_added DESC
    LIMIT  40  "
    ;
                    
    $places$this->db->query($query);
                    foreach (
    $places->result() as $row)
                        {
                            
    $this->place_results[]= array('AID'=>$row->AID,'title'=>$row->title'city'=>$row->cn'date_added'=>$row->date_added
                                                      
    'suburb'=>$row->suburb);
                        }
                    return 
    $this->place_results;
                }
                
            
            function 
    place_details($ad_id)
                {
                    
    $query " SELECT ad_have.id AS AID,
           title,
           comments,
           date_added,
           suburb,
           city,
           city.name  AS cn,
           suburb,
           weekly_rent,
           ad_have.smoke,
           ad_have.views,
           security_bond,
           furnished,
           building_type,
           washer_dryer,
           tv,
           pet,
           carpark,
           internet,
           own_bathroom,
           aircondition,
           address
    FROM   ad_have
           INNER JOIN city
                   ON city.id = ad_have.city
           INNER JOIN members
                   ON members.id = ad_have.member_id
    WHERE  ad_have.id =?
           AND active = ?  "
    ;
                    
    $result $this->db->query($query, array($ad_id'y'));
                    if(
    $result->num_rows()!=1)
                        {
                            return 
    false;
                        }    
                    else 
                        {
                            
    $details $result->result();
                            
    $this->ad_details = array('cn'=>$details[0]->cn,
                                    
    'comments'=>$details[0]->comments,
                                    
    'AID'=>$details[0]->AID,
                                    
    'title'=>$details[0]->title,
                                    
    'city'=>$details[0]->cn,
                                    
    'date_added'=>$details[0]->date_added,
                                    
    'weekly_rent'=>$details[0]->weekly_rent,
                                    
    'smoke'=>$details[0]->smoke,
                                    
    'suburb'=>$details[0]->suburb,
                                    
    'security_bond'=>$details[0]->security_bond,
                                    
    'views'=>$details[0]->views,
                                    
    'furnished'=>$details[0]->furnished,
                                    
    'washer_dryer'=>$details[0]->washer_dryer,
                                    
    'tv'=>$details[0]->tv,
                                    
    'carpark'=>$details[0]->carpark,
                                    
    'internet'=>$details[0]->internet,
                                    
    'own_bathroom'=>$details[0]->own_bathroom,
                                    
    'aircondition'=>$details[0]->aircondition,
                                    
    'address'=>$details[0]->address,
                                    
    'building_type'=>$details[0]->building_type,
                                    
    'pet'=>$details[0]->pet
                                    
    );
                            return 
    $this->ad_details;
                        }
                }
                
                
            
                
        }
    Controller
    PHP Code:
    <?php if ( ! defined('BASEPATH')) exit('No direct script access allowed');

    class 
    Ad_details extends CI_Controller {

        public function 
    show_ad($ad_id)
            {
                if(
    is_numeric($ad_id))
                    {
                        
    $this->load->model('places_model');
                        
    $place $this->places_model->place_details($ad_id);
                        if(
    $place)
                            {
                                
    $data['title'] = html_entity_decode(ucwords($place['title']));
                                if(
    $place['furnished']=='on')
                                    {
                                        
    $furnished "Furnished";
                                    }
                                else 
                                    {
                                        
    $furnished "";
                                    }
                                if(
    $place['washer_dryer']=='on')
                                    {
                                        
    $washer_dryer "Washer / Dryer";
                                    }
                                else
                                    {
                                        
    $washer_dryer "";
                                    }
                                if(
    $place['tv']=='on')
                                    {
                                        
    $tv "TV available";
                                    }
                                else
                                    {
                                        
    $tv "";
                                    }
                                if(
    $place['pet']=='on')
                                    {
                                        
    $pet "Pets Allowed";
                                    }
                                else
                                    {
                                        
    $pet "";
                                    }
                                if(
    $place['carpark']=='on')
                                    {
                                         
    $carpark "Carpark Available";
                                    }
                                else
                                    {
                                        
    $carpark "";
                                    }
                                if(
    $place['internet']=='on')
                                    {
                                        
    $internet "ADSL Available";
                                    }
                                else
                                    {
                                        
    $internet "";
                                    }
                                if(
    $place['own_bathroom']=='on')
                                    {
                                        
    $own_bathroom "Own Bathroom";
                                    }
                                else
                                    {
                                        
    $own_bathroom "";
                                    }
                                if(
    $place['aircondition']=='on')
                                    {
                                        
    $aircondition "Airconditioned";
                                    }
                                else
                                    {
                                        
    $aircondition "";
                                    }
                            
                                
    $data['details'][] = array('AID'=>$place['AID'],
                                
    'title'=>html_entity_decode($place['title']), 
                                
    'comments'=>html_entity_decode($place['comments']), 
                                
    'date_added'=>($place['date_added']), 
                                
    'suburb'=>html_entity_decode($place['suburb']), 
                                
    'city'=>html_entity_decode($place['city']),
                                
    'suburb'=>html_entity_decode($place['suburb']),
                                
    'cn'=>html_entity_decode($place['cn']),
                                
    'security_bond'=>html_entity_decode($place['security_bond']),
                                
    'building_type'=>html_entity_decode($place['building_type']),
                                
    'weekly_rent'=>html_entity_decode($place['weekly_rent']),
                                
    'address'=>html_entity_decode($place['address']),
                                
    'furnished'=>$furnished,
                                
    'washer_dryer'=>$washer_dryer,
                                
    'tv'=>$tv,
                                
    'pet'=>$pet,
                                
    'carpark'=>$carpark,
                                
    'internet'=>$internet,
                                
    'own_bathroom'=>$own_bathroom,
                                
    'aircondition'=>$aircondition,
                                );
                                
    $this->load->vars($data);
                                
    $this->view_things();
                            }
                        else 
                            {
                                
    $this->invalid_ad();
                            }
                    }
                else 
                    {
                        
    $this->invalid_ad();
                    }
            }
        
            
        public function 
    invalid_ad()
            {
                
    $data['invalid']=true;
                
    $data['title']='Flatmatescenter invalid Ad id!';
                
    $this->load->vars($data);
                
    $this->load->view('header_view');
                
    $this->load->view('invalid_ad_details_view');
                
    $this->load->view('footer_view');
            }
            
            
        public function 
    view_things()
            {
                
    $this->load->view('header_view');
                
    $this->load->view('ad_details_view');
                
    $this->load->view('footer_view');
            }
    }

    /* End of file welcome.php */
    /* Location: ./application/controllers/welcome.php */
    View
    PHP Code:
    <div id="common_div" >
    <?php 
    foreach($details as $val)
        {
        
    $title ucwords($val['title']);
        
    $date_added "<div class=\"list\">Listed On ".date('l, jS F Y',strtotime($val['date_added']))."</div>";
        
    $city ucwords($val['city']);
        
    $suburb ucwords($val['suburb']);
        
    $comments ucwords($val['comments']);
        
    $weekly_rent "<div class=\"list\">Weekly Rent $".$val['weekly_rent']."</div>";
        
    $furnished "<div class=\"list\">It is ".$val['furnished']."</div>";
        
    $security_bond "<div class=\"list\">Security Bond $".$val['security_bond']."</div>";
        
    $internet "<div class=\"list\">".$val['internet']."</div>";
        
    $carpark "<div class=\"list\">".$val['carpark']."</div>";
        
    $tv "<div class=\"list\">".$val['tv']."</div>";
        
    $pet "<div class=\"list\">".$val['pet']."</div>";
        
    $aircondition "<div class=\"list\">".$val['aircondition']."</div>";
        
    $building_type "<div class=\"list\">".$val['building_type']."</div>";
        
    $washer_dryer "<div class=\"list\">".$val['washer_dryer']."</div>";
        
    $own_bathroom "<div class=\"list\">".$val['own_bathroom']."</div>";
        
    $address "<div class=\"list\">".$val['address']."</div>";
        }
    ?>
    <div class = "about">
        <span style="color:yellow"><?php echo $title;?></span> in <?php echo $city.", ".$suburb;?>
        <a href = "<?php echo site_url();?>" style="float:right; margin:0; clear:both;">Back To Listings</a>
    </div>
    <?php
    echo "<div class = \"place_details\" style =\"float:none; clear:both\">";
    echo 
    $date_added;
    echo 
    $weekly_rent;
    echo 
    $furnished;
    echo 
    $internet;
    echo 
    $pet;
    echo 
    $carpark;
    echo 
    $own_bathroom;
    echo 
    $aircondition;
    echo 
    $washer_dryer;
    echo 
    $building_type;
    echo 
    $address;
    echo 
    $security_bond;
    echo 
    "<div style =\" float:none; clear:both\"></div>";
    echo 
    "</div>";
        
            echo 
    "<div class = \"places\" style=\"clear:both; font-size:14px; line-height:25px;\">";
            echo 
    $comments;
            echo 
    "</div>";
            
        
        
    ?>
    </div>
  2. #2
  3. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,957
    Rep Power
    1046
    Hi,

    using html_entity_decode() in this context is wrong and makes your script vulnerable to any HTML injection. This function will decode HTML entities (like the name says), so for example "& lt;" will become "<". But you want the exact opposite, namely encoding characters to HTML entities.

    Also I don't see why you would use this oldschool PHP/HTML spaghetti code for your templates when at the same time you do all this fancy OOP to get a clean structure. I'd use a template engine like Smarty or Twig.
  4. #3
  5. Mad Scientist
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Oct 2007
    Location
    North Yorkshire, UK
    Posts
    3,661
    Rep Power
    4123
    Template engines are a bit of a sticking point for me

    PHP is a templating engine, so I see no problems with having spaghetti code in the view, in this context anyway.

    smarty is just spaghetti code with smarty flavoured source rather than php flavoured source. It adds untold layers of complexity, extra processing and is far less portable (I've ni)

    However, spaghetti code can be a mess, and a nightmare to manage, so consider making the views modular for maintainability.

    I try to split my applications into two separate applications, a data api which could equate to the model and controller parts of MVC and separately a formatting part which calls the api and formats the data for output (this could equate to the view part of MVC). These two parts can then be wrapped up behind a "front controller".

    more often than not my applications only ever output JSON, for websites I use MODx, openCart or a combination of the two.

    IMO, MODx has the best templating engine ever - the direction of data is reversed, so the template makes mini requests to get the data, and can be formatted at a very granular level....or quite macroscopically
    I said I didn't like ORM!!! <?php $this->model->update($this->request->resources[0])->set($this->request->getData())->getData('count'); ?>

    PDO vs mysql_* functions: Find a Migration Guide Here

    [ Xeneco - T'interweb Development ] - [ Are you a Help Vampire? ] - [ Read The manual! ] - [ W3 methods - GET, POST, etc ] - [ Web Design Hell ]
  6. #4
  7. A Change of Season
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Mar 2004
    Location
    Next Door
    Posts
    2,650
    Rep Power
    171
    Hello and thank you for your replies;

    Template engines aside, is the rest of the code design ok? It is very important for me to know I am doing it the right way. I am building a lot of websites at this stage, I don't want to make thinks in the wrong way.
    Also a few questions:
    1 - htmlentities functions are best to be applied in controller? Or in model? Or view?
    2 - Even though it adds untold layers of complexity, extra processing and is far less portable, using template engines is a good idea. Is it true?
    3 - Is there any other ways to make the controller shorter? You see all the logic there (if else....). I don't like that.
    Thanks again
  8. #5
  9. Mad Scientist
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Oct 2007
    Location
    North Yorkshire, UK
    Posts
    3,661
    Rep Power
    4123
    I wouldn't encode html entities at all

    I would use html_entity_decode in the view, only if my view was rendering html

    PHP is a templaing engine, unless you have good reason to implement a less version of its self within its self then I'd say just use PHP

    I would structure my controller differently...how, exactly, depends on what you're trying to achieve
    I said I didn't like ORM!!! <?php $this->model->update($this->request->resources[0])->set($this->request->getData())->getData('count'); ?>

    PDO vs mysql_* functions: Find a Migration Guide Here

    [ Xeneco - T'interweb Development ] - [ Are you a Help Vampire? ] - [ Read The manual! ] - [ W3 methods - GET, POST, etc ] - [ Web Design Hell ]
  10. #6
  11. Contributing User
    Devshed Novice (500 - 999 posts)

    Join Date
    Sep 2002
    Location
    Seattle, U.S.A.
    Posts
    712
    Rep Power
    12
    Originally Posted by zxcvbnm
    Hello and thank you for your replies;

    Template engines aside, is the rest of the code design ok? It is very important for me to know I am doing it the right way. I am building a lot of websites at this stage, I don't want to make thinks in the wrong way.
    Also a few questions:
    1 - htmlentities functions are best to be applied in controller? Or in model? Or view?
    2 - Even though it adds untold layers of complexity, extra processing and is far less portable, using template engines is a good idea. Is it true?
    3 - Is there any other ways to make the controller shorter? You see all the logic there (if else....). I don't like that.
    Thanks again
    1. I would say it depends on the context of what you are doing when to put it in controller or the view. You should not put it in the model.

    2. Since you are using Codeigniter, then you don't really need a templating layer. It already has views for you to use which is really a templating layer.

    3. You could probably figure out a way to put more of the on off stuff in your view.

    <?php echo $place['furnished'] == 'on' : 'html code' : '' ?>

IMN logo majestic logo threadwatch logo seochat tools logo