Is this well written php?

Dev Shed Forums Sponsor:

November 23rd, 2012, 08:23 PM

zxcvbnm

A Change of Season

Join Date: Mar 2004

Posts: 1,589

Time spent in forums: 2 Weeks 4 Days 19 h 37 m 37 sec
Reputation Power: 70

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 :-)

November 24th, 2012, 01:04 AM

Jacques1

pollyanna

Join Date: Jul 2012

Location: Germany

Posts: 1,843

Time spent in forums: 1 Month 2 Weeks 1 Day 7 h 32 m 15 sec
Reputation Power: 813

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.

November 24th, 2012, 06:24 PM

zxcvbnm

A Change of Season

Join Date: Mar 2004

Posts: 1,589

Time spent in forums: 2 Weeks 4 Days 19 h 37 m 37 sec
Reputation Power: 70

Quote:

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'])));

Code:

{$places[list].title|htmlentities}

Quote:

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.

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?

Quote:

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.

Quote:

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

Quote:

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.

November 24th, 2012, 08:26 PM

Jacques1

pollyanna

Join Date: Jul 2012

Location: Germany

Posts: 1,843

Time spent in forums: 1 Month 2 Weeks 1 Day 7 h 32 m 15 sec
Reputation Power: 813

Quote:

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.

Quote:

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.

Quote:

Originally Posted by zxcvbnm

Quote:

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.

Quote:

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".

November 25th, 2012, 06:41 PM

zxcvbnm

A Change of Season

Join Date: Mar 2004

Posts: 1,589

Time spent in forums: 2 Weeks 4 Days 19 h 37 m 37 sec
Reputation Power: 70

Quote:

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

November 26th, 2012, 05:56 AM

annaharris

Registered User

Join Date: Mar 2012

Location: USA

Posts: 33

Time spent in forums: 8 h 1 m 35 sec
Reputation Power: 0

Also i agree with Jacques that your code is really fine. So you no need to worry about that.

November 26th, 2012, 09:15 AM

Northie

Square Peg in a Round Hole

Join Date: Oct 2007

Location: North Yorkshire, UK

Posts: 3,415

Time spent in forums: 3 Weeks 5 Days 9 h 21 m 40 sec
Reputation Power: 3833

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

__________________
PHP OOPS! <?php DB::Execute(SQL::makeFrom($_GET))->fetchArray()->FormatWith(Template::getInstance('default'))->printHtml(); ?>

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 ]

November 27th, 2012, 05:14 AM

inetweaver

Registered User

Join Date: Nov 2012

Posts: 5

Time spent in forums: 27 m 20 sec
Reputation Power: 0

Thanks for sharing

Is this well written php?

Developer Shed Advertisers and Affiliates