Code Critique Please?

Page 1 of 1

11 Replies - 743 Views - Last Post: 10 September 2012 - 02:44 AM Rate Topic: -----

#1 Rabastan  Icon User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 22
  • Joined: 27-August 12

Code Critique Please?

Posted 08 September 2012 - 07:52 AM

OK so this is my first attempt to write my own php class. Can I get a critique on what I have so far. The idea is to dynamically generate an html header,while loading all of the css and javascript files in there respective directories. Here is what I have

In "app.php" which is included at top of every page
<?php
include('app/config.php');
require(DIR_ENGLISH . 'general.php');
?>


in "english/general.php"
<?php
define('SITE_DOCTYPE', '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">');
	define('SITE_TITLE', 'Rabs Website' );
	define('SITE_META', '<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />');
	define('SITE_DESC', '<meta name="description" content="Description To Be Added Later">');
	define('SITE_KEYWORDS', '<meta name="keywords" content="Keywords to be added later">');
?>


and in the main class I have:
<?php
class html_head{
	public $type;
	public $title;
	public $meta;
	public $desc;
	public $keywords;
	public $styles;
	public $scripts;

	public function setType($type){
			$this->type=SITE_TYPE;
	}
	public function setTitle($title){
			$this->title=SITE_TITLE;
	}
	public function setMeta($meta){
			$this->meta=SITE_META;
	}
	public function setDesc($desc){
			$this->desc=SITE_DESC;
	}
	public function setKeywords($keywords){
			$this->keywords=SITE_KEYWORDS;
	}
	function loadSystemCSS($styles)
{
  $includes = '';
  if ($css_dir = @dir(styles . 'css')) {
    while ($css_file = $css_dir->read()) {
    if (preg_match('/^[A-Za-z0-9_-]+\\.css$/', $css_file) > 0) {
      $includes .= '<link rel="stylesheet" type="text/css" href="css/'.$css_file.'" />' . "\\n";
    }
    }
    $css_dir->close();
  }
  return $includes;
}
function loadSystemJS($scripts)
{
  $includes = '';
  if ($js_dir = @dir($scripts . 'js')) {
    while ($js_file = $js_dir->read()) {
    if (preg_match('/^[A-Za-z0-9_-]+\\.js$/', $js_file) > 0) {
      $includes .= '<script type="text/javascript" src="js/'.$js_file.'"></script>' . "\\n";
    }
    }
    $js_dir->close();
  }
  return $includes;
}
}
?>


I have yet to build the __constructor yet just want to make sure I am good up to this point.

thanks
Rab

Is This A Good Question/Topic? 0
  • +

Replies To: Code Critique Please?

#2 CTphpnwb  Icon User is offline

  • D.I.C Lover
  • member icon

Reputation: 2486
  • View blog
  • Posts: 8,533
  • Joined: 08-August 08

Re: Code Critique Please?

Posted 08 September 2012 - 08:10 AM

You're passing parameters to your methods (i.e., setTitle($title)) but then you're using constants instead of the parameters.

Since you're using setters and getters, don't make your variables public, make them protected.
Was This Post Helpful? 1
  • +
  • -

#3 codeprada  Icon User is online

  • Changed Man With Different Priorities
  • member icon

Reputation: 934
  • View blog
  • Posts: 2,332
  • Joined: 15-February 11

Re: Code Critique Please?

Posted 08 September 2012 - 08:13 AM

Hey, if you look at your setter methods (setType, ..., setKeywords), I noticed you're passing a parameter to them but setting the associated property to your constant.

Refrain from using the @ operator. Instead set the error within an error property so that it can be accessed from outside of the class.

Example:
<?php
	class Test
	{
		public $error = NULL;
		
		public function someMethod()
		{
			// do something here
			if($operation_failed)
				$this->error = "Method failed because.....";
			else
				$this->error = NULL;
			
			//return's either true or false. false let's you know that an error occurred since $error will contain the error string
			return is_null($this->error);
		}
	}
?>

Was This Post Helpful? 0
  • +
  • -

#4 Rabastan  Icon User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 22
  • Joined: 27-August 12

Re: Code Critique Please?

Posted 08 September 2012 - 08:13 AM

So other than that does it look good??

Rab
Was This Post Helpful? 0
  • +
  • -

#5 Rabastan  Icon User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 22
  • Joined: 27-August 12

Re: Code Critique Please?

Posted 08 September 2012 - 08:20 AM

View Postcodeprada, on 08 September 2012 - 08:13 AM, said:

Hey, if you look at your setter methods (setType, ..., setKeywords), I noticed you're passing a parameter to them but setting the associated property to your constant.

Refrain from using the @ operator. Instead set the error within an error property so that it can be accessed from outside of the class.

Example:
<?php
	class Test
	{
		public $error = NULL;
		
		public function someMethod()
		{
			// do something here
			if($operation_failed)
				$this->error = "Method failed because.....";
			else
				$this->error = NULL;
			
			//return's either true or false. false let's you know that an error occurred since $error will contain the error string
			return is_null($this->error);
		}
	}
?>


I don't get what your saying here, sorry.

Rab
Was This Post Helpful? 0
  • +
  • -

#6 codeprada  Icon User is online

  • Changed Man With Different Priorities
  • member icon

Reputation: 934
  • View blog
  • Posts: 2,332
  • Joined: 15-February 11

Re: Code Critique Please?

Posted 08 September 2012 - 08:40 AM

If there's an error in your method then instead of suppressing it with the @ operator you can simply get it with
$test = new Test();
if(!$test->someMethod()) //if it returns false then it failed
    echo $test->error; //view your error here

//better error handling. never use the @ operator


Have a look at this tutorial also: Making Your Site More User-Friendly By Handling Errors Correctly
Was This Post Helpful? 1
  • +
  • -

#7 Rabastan  Icon User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 22
  • Joined: 27-August 12

Re: Code Critique Please?

Posted 08 September 2012 - 09:51 AM

ok Now I get it, thank you. I am building this site to learn PHP. So writing better code is important to me.

Rab
Was This Post Helpful? 0
  • +
  • -

#8 Rabastan  Icon User is offline

  • New D.I.C Head

Reputation: 0
  • View blog
  • Posts: 22
  • Joined: 27-August 12

Re: Code Critique Please?

Posted 08 September 2012 - 10:22 AM

Codeprada,

OK, so I get exactly what your telling me to do and why. I can't seem to get it to work in my code without my IDE throwing an error. I am pretty sure it has something to do my brackets, which I will be the first to admit I have an issue with.

I know someone is going to mention that I didn't post my code, I need to explain that I failed to do so on purpose. I like to try and figure it out (at least until I am so frustrated my puter is at of being smashed. Its how I learn). I would much appreciate it if you would point me in the right direction as to how to place the brackets when incorporating it into my code.

Rab
Was This Post Helpful? 0
  • +
  • -

#9 Sho Ke  Icon User is offline

  • D.I.C Head
  • member icon

Reputation: 97
  • View blog
  • Posts: 230
  • Joined: 13-October 11

Re: Code Critique Please?

Posted 08 September 2012 - 11:43 AM

It kind of sounds like you're wanting an indent style.

There are plenty of indent styles out there, and none are necessarily better than others. It's all about how you(and possibly other people that will work with your code) can read and understand it.

For example, I'd prefer something that looks like

			if(x) {
				if(y) {
					//code to execute if x and y are true
				}
				else {
					//code to execute if x is true but not y
				}
			}
			elseif(y) {
				//code to execute if y is true but not x
			}
			else {
				//code to execute if neither is true
			}


But the guy next to me in class usually writes something like this:

			if(x)
			{
				if(y)
				{
					//code to execute if x and y are true
				}
				else
				{
					//code to execute if x is true but not y
				}
			}
			elseif(y)
			{
				//code to execute if y is true but not x
			}
			else
			{
				//code to execute if neither is true
			}


The first example isn't better than the last, and the last isn't better than the first. Just pick the one you think is most legible. The important thing is consistency . Pick an indent style and stick with it, else you'll more than likely end up confusing yourself further down the line in a project.
Was This Post Helpful? 2
  • +
  • -

#10 Atli  Icon User is online

  • D.I.C Lover
  • member icon

Reputation: 3052
  • View blog
  • Posts: 4,573
  • Joined: 08-June 10

Re: Code Critique Please?

Posted 08 September 2012 - 08:06 PM

View Postcodeprada, on 08 September 2012 - 03:13 PM, said:

Instead set the error within an error property so that it can be accessed from outside of the class.

I've never really liked this method. Why build this type of error handling into all your classes when PHP has perfectly good Exception handling? - Exceptions can also provide invaluable debug info, which this type of error handling can't really duplicate. Well, not easily in any case.
Was This Post Helpful? 0
  • +
  • -

#11 codeprada  Icon User is online

  • Changed Man With Different Priorities
  • member icon

Reputation: 934
  • View blog
  • Posts: 2,332
  • Joined: 15-February 11

Re: Code Critique Please?

Posted 09 September 2012 - 10:06 AM

View PostAtli, on 08 September 2012 - 11:06 PM, said:

View Postcodeprada, on 08 September 2012 - 03:13 PM, said:

Instead set the error within an error property so that it can be accessed from outside of the class.

I've never really liked this method. Why build this type of error handling into all your classes when PHP has perfectly good Exception handling? - Exceptions can also provide invaluable debug info, which this type of error handling can't really duplicate. Well, not easily in any case.

That's personal preference in my opinion. Some would prefer to just throw the exception and catch it outside of the class while others handle in within the class.
Was This Post Helpful? 0
  • +
  • -

#12 Dormilich  Icon User is offline

  • 痛覚残留
  • member icon

Reputation: 2899
  • View blog
  • Posts: 7,555
  • Joined: 08-June 10

Re: Code Critique Please?

Posted 10 September 2012 - 02:44 AM

View Postcodeprada, on 09 September 2012 - 07:06 PM, said:

Some would prefer to just throw the exception and catch it outside of the class while others handle in within the class.

and what if you cannot handle the error inside the class?
Was This Post Helpful? 0
  • +
  • -
Page 1 of 1