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

    Join Date
    Mar 2004
    Location
    Nobbies beach, Gold Coast. It's beautiful.
    Posts
    2,574
    Rep Power
    171

    Is this well written php?


    Is this well written php? If not what would you change/improve? Thanks



    Controller
    PHP Code:
    <?php if ( ! defined('BASEPATH')) exit('No direct script access allowed');

    class 
    Share_accomodation extends CI_Controller
        
    {
            protected 
    $suburb="";
            
            public function 
    index()
               {
                    
    $this->cities_dropdown();//Load drop down list of cities
                    
    $this->show_ads();//Load list of ads
                    
    $this->view_page();//Load template and view page
                
    }
            protected function 
    cities_dropdown()
                {
                    
    $this->load->model('cities_model');
                    
    $cities $this->cities_model->load_cities();
                    foreach(
    $cities as $val)
                        {
                            
    $australia[] = array('name'=>$val['name'], 'id'=>$val['id'], 'number_of_ads'=>$val['number_of_ads']);
                        }
                    
    $this->smarty->assign("australia"$australia);
                    
    $form_attributes = array('class' => 'form');
                    
    $form_open form_open('search'$form_attributes);
                    
    $form = array('open'=>$form_open'close'=>form_close());
                    
    $this->smarty->assign('form'$form);
                    
    $this->smarty->assign('searched'$this->input->post('search'));
                }
            protected function 
    show_ads()
                {
                    
    $this->load->model('places_model');
                    
    $places $this->places_model->load_places();
                    if(
    $places)
                        {
                            foreach(
    $places as $val)
                                {
                                    if(
    $val['suburb']!=$val['city'])
                                        {
                                            
    $this->suburb ", ".$val['suburb'];
                                        }
                                    else
                                        {
                                            
    $this->suburb="";
                                        }
                                    
    $list_places[] = array('title'=>htmlentities(ucwords(strtolower($val['title']))), 'city_suburb'=>htmlentities(ucwords(strtolower($val['city'].$this->suburb))), 'date_added'=>$val['date_added'], 'AID'=>$val['AID']);
                                }
                        }
                    
    $this->smarty->assign("title""Share accomodation in Australia");
                    
    $this->smarty->assign("places"$list_places);
                }
            
             protected function 
    view_page()
                {
                    
    $this->smarty->view('header');
                    
    $this->smarty->view('places_view');
                } 
        }
    Cities model
    PHP Code:
    <?php if ( ! defined('BASEPATH')) exit('No direct script access allowed');
    class 
    Cities_model extends CI_Model
        
    {
            protected 
    $cities_results;
            public function 
    load_cities()
                {
                    
    $this->db->select('city.name, city.id, COUNT(ad_have.id) AS number_of_ads');
                    
    $this->db->from('city');
                    
    $this->db->join('ad_have''ad_have.city = city.id''left outer');
                    
    $this->db->where('ad_have.active''y');
                    
    $this->db->group_by("city.id"); 
                    
    $this->db->order_by("number_of_ads""desc");                 
                    
    $query $this->db->get();
                    foreach(
    $query->result() as $row)
                        {
                            
    $this->cities_results[]= array('number_of_ads'=>$row->number_of_ads'name'=>$row->name'id'=>$row->id);
                        }
                    return 
    $this->cities_results;
                }
        }
    Places 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()
                {
                    
    $this->db->select('ad_have.id AS AID, title, comments, date_added, suburb, city, city.name AS cn, suburb');
                    
    $this->db->from('ad_have');
                    
    $this->db->join('city''ad_have.city = city.id''inner');
                    
    $this->db->join('members''ad_have.member_id = members.id''inner');
                    
    $this->db->where('ad_have.active''y'); 
                    
    $this->db->order_by("date_added""desc");     
                    
    $query $this->db->get();
                    foreach (
    $query->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 
    search_places($search_id)
                {
                    
    $this->db->select('ad_have.id AS AID, title, comments, date_added, suburb, city, city.name AS cn, suburb');
                    
    $this->db->from('ad_have');
                    
    $this->db->join('city''ad_have.city = city.id''inner');
                    
    $this->db->join('members''ad_have.member_id = members.id''inner');
                    
    $this->db->where('ad_have.city'$search_id); 
                    
    $this->db->order_by("date_added""desc");                 
                    
    $query $this->db->get();
                    foreach(
    $query->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;
                }
        }
    View
    Code:
    <div id="common_div" >
         <div class = "about">Flatmates Centre is 100% FREE
    	 {$form.open}
    		 <select class="australia_drop_down" name = "search">
    				<option value='all' selected="selected">All Australia</option>
    				{section name=australia_drop_down loop=$australia}
    				<option {if $australia[australia_drop_down].id == $searched} selected="selected" {/if} value = "{$australia[australia_drop_down].id}">&nbsp;&nbsp;{$australia[australia_drop_down].name} ({$australia[australia_drop_down].number_of_ads})</option>
    				{/section}
    		</select>
    	{$form.close}
    	 </div>
         <div class = "places">
         <div class="ad_title"><strong>Ad Title</strong></div><span class="ad_date">
         <strong>Date Added</strong></span><span class= "city"><strong>City and Suburb</strong></span><span class= "view">
         <strong>View</strong></span>
         </div>
         {if isset($places)}
    		 {counter start=0 print=false name=bla}
    		 {section name=list loop=$places}
    			  <div class = "places" >
    			  <div class="ad_title"><a href="#" class="add_title_link">{counter} - {$places[list].title}</a></div>
    			  <span class="ad_date">{date('l, jS F Y',strtotime($places[list].date_added))}</span>
    			  <span class= "city">{$places[list].city_suburb}</span><span style="float:right; ">
    			  <a class= "link_view" href="ad_details/show_ad/{$places[list].AID}">View Details</a>
    			  </span><span style="clear:both;"></span>
    			  </div>
    		 {/section}
    		{else}
    		<div class = "places" >Sorry your search returned no results!</div>
    		{/if}
    </div>
    header view
    Code:
    <!DOCTYPE html>
    <html>
    <head>
        <meta charset="utf-8">
        <title>{$title}</title>
    	<link type="text/css" href="default.css" rel="stylesheet" /> 
        <script src="http://code.jquery.com/jquery-latest.min.js"></script>
    	<script type="text/javascript" src="all_javascripts.js"></script>
    </head>
    <body>

    Comments on this post

    • Jacques1 agrees : nice code :-)
  2. #2
  3. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,920
    Rep Power
    1045
    Hi,

    I find it great that you want to learn and improve your code. You really have a lot of ambition.

    But don't ask for our blessing whenever you write a line of code.

    Your code is fine, so no need to worry about having done something "wrong". Yeah, maybe one of the "PHP gurus" around here still finds something he doesn't like if he looks really, really hard. So what? Would that make your code bad or your users unhappy?

    You've obviously understood the MVC pattern, the rest is just details. And there isn't a definite "right" in "wrong" in programming, anyway.

    Don't get me wrong: It does make sense to ask more experienced programmers about their opinion on a specific idea or solution. But don't expect a forum to do your daily code review. That's just too much.

    However, I do have some remarks regarding the templates:

    Smarty does not free you from escaping values. When you just use {$var}, you'll have the same security problems like with <?php echo $var ?>. You always have to e. g. apply the "htmlentities" modifier:
    Code:
    {$var|htmlentities}
    It's also possible to define default modifiers that will be applied to all variables automatically.

    And you should avoid the cumbersome "section" syntax (unless there's a special reason why you need it). Use the "foreach" syntax instead.
    Last edited by Jacques1; November 24th, 2012 at 01:12 AM.
  4. #3
  5. A Change of Season
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Mar 2004
    Location
    Nobbies beach, Gold Coast. It's beautiful.
    Posts
    2,574
    Rep Power
    171
    Originally Posted by Jacques1
    Smarty does not free you from escaping values. When you just use {$var}, you'll have the same security problems like with <?php echo $var ?>. You always have to e. g. apply the "htmlentities" modifier:
    Code:
    {$var|htmlentities}
    It's also possible to define default modifiers that will be applied to all variables automatically.
    Umm, if you see the the controller in my code above, I was using htmlentities there in my controller. Which brings us to another discussion: Is it better to use htmlentities in controller or in template?
    PHP Code:
    $list_places[] = array('title'=>htmlentities(ucwords(strtolower($val['title']))); 
    VS
    Code:
    {$places[list].title|htmlentities}
    Originally Posted by Jacques1
    Hi,

    I find it great that you want to learn and improve your code. You really have a lot of ambition.

    But don't ask for our blessing whenever you write a line of code.

    Your code is fine, so no need to worry about having done something "wrong". Yeah, maybe one of the "PHP gurus" around here still finds something he doesn't like if he looks really, really hard. So what? Would that make your code bad or your users unhappy?
    I wasted months doing what I thought was the right way. There was a point I realized there are many ways things can be done in php but a lot of those methods could be wrong. I am self taught and havent done schooling on this, I realized checking here saves me loads of time. Remember you told me about Smarty?
    Originally Posted by Jacques1
    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.
    It is adding a whole new skill sets to my list. Now when I look for php jobs I see Smarty I know what it's talking about.
    Originally Posted by Jacques1
    And there isn't a definite "right" in "wrong" in programming, anyway.
    O man,, there is. Trust me. I have done some code out of ignorance and wrong assumptions you won't believe
    Originally Posted by Jacques1
    But don't expect a forum to do your daily code review. That's just too much.
    I've almost always got something out of every thread I posted. I really don't see much harm. But sure, I hear you and try to post only if I have to.

    Thanks again, your replies are very very helpful.
  6. #4
  7. --
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Jul 2012
    Posts
    3,920
    Rep Power
    1045
    Originally Posted by zxcvbnm
    Umm, if you see the the controller in my code above, I was using htmlentities there in my controller.
    OK, that one htmlentities() I didn't see. But they should be applied to any value that goes into the template, regardless of whether or not it's actually "dangerous" right now.



    Originally Posted by zxcvbnm
    Which brings us to another discussion: Is it better to use htmlentities in controller or in template?
    I'd put it in the templates, because then you can actually see if every value is escaped. It's easier to forget that in the controller.

    But if you want to do all the data preparation in the controller and keep the templates "stupid", you might as well put it in the controller.



    Originally Posted by zxcvbnm
    Originally Posted by Jacques1
    And there isn't a definite "right" in "wrong" in programming, anyway.
    O man,, there is. Trust me. I have done some code out of ignorance and wrong assumptions you won't believe
    Well, I did exaggerate a bit. Of course there are many problems where one solution is definitely better than another. But when you've reached a certain level of programming techniques and begin to deal with design details, often enough there is no "best solution". One approach might be slightly faster, the next one is a bit more readable etc. -- so it basically depends on one's own preferences

    Regarding the htmlentities() question, for example, I think that's really a matter of taste.



    Originally Posted by zxcvbnm
    I've almost always got something out of every thread I posted. I really don't see much harm. But sure, I hear you and try to post only if I have to.
    I'm really not suggesting that you should post less often or something. I just got the impression that you sometimes worry too much about doing something "wrong".
  8. #5
  9. A Change of Season
    Devshed Frequenter (2500 - 2999 posts)

    Join Date
    Mar 2004
    Location
    Nobbies beach, Gold Coast. It's beautiful.
    Posts
    2,574
    Rep Power
    171
    Originally Posted by Jacques1
    OK, that one htmlentities() I didn't see. But they should be applied to any value that goes into the template, regardless of whether or not it's actually "dangerous" right now.





    I'd put it in the templates, because then you can actually see if every value is escaped. It's easier to forget that in the controller.

    But if you want to do all the data preparation in the controller and keep the templates "stupid", you might as well put it in the controller.





    Well, I did exaggerate a bit. Of course there are many problems where one solution is definitely better than another. But when you've reached a certain level of programming techniques and begin to deal with design details, often enough there is no "best solution". One approach might be slightly faster, the next one is a bit more readable etc. -- so it basically depends on one's own preferences

    Regarding the htmlentities() question, for example, I think that's really a matter of taste.





    I'm really not suggesting that you should post less often or something. I just got the impression that you sometimes worry too much about doing something "wrong".
    Thank you Jacques1, see you in the next threads
  10. #6
  11. No Profile Picture
    Registered User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Mar 2012
    Location
    USA
    Posts
    33
    Rep Power
    0
    Also i agree with Jacques that your code is really fine. So you no need to worry about that.
  12. #7
  13. Mad Scientist
    Devshed Expert (3500 - 3999 posts)

    Join Date
    Oct 2007
    Location
    North Yorkshire, UK
    Posts
    3,660
    Rep Power
    4123
    I'd omit this fluff

    PHP Code:
    if ( ! defined('BASEPATH')) exit('No direct script access allowed'); 
    as your files seem to only contain classes then direct access to the script would cause no execution at all

    If PHP was some how disabled then the raw source would be viewable whether that line was in or not - because that line wouldn't run because PHP was disabled.

    a cleaner way, IMO, is to keep library files outside the web root
    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 ]
  14. #8
  15. No Profile Picture
    Registered User
    Devshed Newbie (0 - 499 posts)

    Join Date
    Nov 2012
    Posts
    5
    Rep Power
    0
    Thanks for sharing

IMN logo majestic logo threadwatch logo seochat tools logo