First Ever Class - Need Critique on style and syntax

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
User avatar
Inkyskin
Forum Contributor
Posts: 282
Joined: Mon Nov 19, 2007 10:15 am
Location: UK

First Ever Class - Need Critique on style and syntax

Post by Inkyskin »

Hi all,

I have just put together my first useful class (To me anyhow) - I was wondering where I could improve on syntax etc

You call the connect_to_article function when using it, the rest does what it needs by itself:

Code: Select all

<?
class articles_controller {
	
	//
	// Base Variables (Story)
	//
	var $id;
	var $comments;
	var $type;
	var $type_text;
	var $type_link;
	var $views_before;
	var $views_after;
	var $title;
	var $seo_path;
	var $seo_title;
	var $main_body;
	var $summary;
	//
	// Base Variables (Sizing)
	//
	var $pro_account;
	var $story_width;
	var $show_right_adverts;
	var $text_box_width;
	var $comments_width;
	
	function article_type($type_id){
		//
		// Set the article type info
		//
		switch ($type_id) {
			case 1:
				$this->type      = 1;
				$this->type_text = 'News';
				$this->type_link = 'news';
			  break;
			case 2:
				$this->type      = 2;
				$this->type_text = 'Reviews';
				$this->type_link = 'reviews';
			  break;
			case 3:
				$this->type      = 3;
				$this->type_text = 'Articles';
				$this->type_link = 'articles';
			  break;
			case 4:
				$this->type      = 4;
				$this->type_text = 'Photography Guides';
				$this->type_link = 'guides';
			  break;
		}
	}
	
	function layout_management($member_level){
		//
		// Set layout vars depending on ads showing or not
		//
		switch ($member_level) {
			case 3:
				$this->story_width        = 'full_col';
				$this->show_right_adverts = false;
				$this->comments_width     = '645';
				$this->text_box_width     = '718';
				break;
			default:
				$this->story_width        = 'two_thirds_col';
				$this->show_right_adverts = true;
				$this->comments_width     = '485';
				$this->text_box_width     = '557';
				break;
		}
	}
	
	function connect_to_article($seo_path,$article_type,$account_type){
		//
		// Get content
		//
		$get_article_data_sql   = "SELECT stories_and_articles_id,total_comments,seo_title,story_title,short_summary,story_body,story_hits FROM stories_and_articles WHERE friendly_url_text = '".$seo_path."'";
		$get_article_data_query = sp_query($get_article_data_sql,'Artices Controller: Get Article\'s Data');
		$row_articles_data      = mysql_fetch_array($get_article_data_query);
		//
		// Assign base variables
		//
		$this->id           = $row_articles_data[stories_and_articles_id];
		$this->comments     = $row_articles_data[total_comments];
		$this->seo_title    = $row_articles_data[seo_title];
		$this->title        = $row_articles_data[story_title];
		$this->summary      = $row_articles_data[short_summary];
		$this->main_body    = $row_articles_data[story_body];
		$this->views_before = $row_articles_data[story_hits];
		$this->type         = $article_type;
		$this->seo_path     = $seo_path;
		//
		// Update views
		//
		$this->update_article_views();
		//
		// Update article type
		//
		$this->article_type($article_type);
		//
		//
		//
		$this->layout_management($account_type);
		//
		// Fill array and return
		//
		$data_array = Array(
			'id'                  => $this->id,
			'comments'            => $this->comments,
			'type'                => $this->type,
			'type_text'           => $this->type_text,
			'type_link'           => $this->type_link,
			'views_before'        => $this->views_before,
			'views_after'         => $this->views_after,
			'title'               => $this->title,
			'seo_path'            => $this->seo_path,
			'seo_title'           => $this->seo_title,
			'main_body'           => $this->main_body,
			'summary'             => $this->summary,
			'story_width'         => $this->story_width,
			'show_right_adverts'  => $this->show_right_adverts,
			'comments_width'      => $this->comments_width,
			'text_box_width'      => $this->text_box_width
		);
		//
		// Return our data
		//
		return $data_array;
	}
	
	function update_article_views(){
		//
		// Update view count
		//
		$this->views_after  = $this->views_before + 1;
		$update_sql         = "UPDATE stories_and_articles SET story_hits = ".$this->views_after." WHERE stories_and_articles_id = ".$this->id;
		$update_query       = sp_query($update_sql,'Articles Controller: Update Article\'s Views');
	}

}
?>
Thanks :D
Last edited by Inkyskin on Thu Dec 27, 2007 3:10 pm, edited 1 time in total.
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

Some quick "good practices" notes:

- Use $row_articles_data['stories_and_articles_id'] not $row_articles_data[stories_and_articles_id]
- connect_to_article() looks dangerous, since $seo_path isn't being escaped. Look into using a DB abstraction library/bound parameters
- There's no documentation on what $member_level indicates. If you're using magic numbers, use constants instead. Same goes for article types
- You're mixing presentation and model (i.e. how the data is displayed and how you are retrieving/processing the data). The class is basically class-ified procedural and doesn't really take advantage of OOP (besides the fact that you are using $this, which is a good first step, but you can do a lot more)
User avatar
Inkyskin
Forum Contributor
Posts: 282
Joined: Mon Nov 19, 2007 10:15 am
Location: UK

Post by Inkyskin »

Thanks for the input :)

There is more that will be added in the future, such as adding comments etc, which is why I chose to go with a class. It's more to have everything in one place, with as few calls for more information as possible.

- I never thought about escaping the seo_path before - nice catch, could have had a nasty problem there!
- Member level is described in the database, and in my main config file, but I suppose it wouldnt hurt to add here to.
Use $row_articles_data['stories_and_articles_id'] not $row_articles_data[stories_and_articles_id]
Is there any benefit to doing this? I've seen it done both ways and never personally had a problem with it...
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

Using the unquoted version will throw E_NOTICE's and will cause problems if a constant of that name is ever defined. It's good practice: quote!
Member level is described in the database, and in my main config file, but I suppose it wouldnt hurt to add here to.
Why do that when you can make it explicit? (besides, then you'd have it in multiple places where you'd have to update it every time)
User avatar
Inkyskin
Forum Contributor
Posts: 282
Joined: Mon Nov 19, 2007 10:15 am
Location: UK

Post by Inkyskin »

It's good practice: quote!
True!
Why do that when you can make it explicit? (besides, then you'd have it in multiple places where you'd have to update it every time)
Good point, I'll probably look into straigtning that out
Post Reply