Page 1 of 1

First Ever Class - Need Critique on style and syntax

Posted: Wed Dec 26, 2007 2:05 pm
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

Posted: Wed Dec 26, 2007 2:20 pm
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)

Posted: Wed Dec 26, 2007 2:33 pm
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...

Posted: Wed Dec 26, 2007 2:35 pm
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)

Posted: Wed Dec 26, 2007 2:41 pm
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