Please review my Basic php OOP code

[irkevin]

Hi guys,

 

I'm new to OOP and i would be grateful if someone could review the simple and basic code i wrote and tell me if the technique is good or if i can better it.

 

What I did was a simple class file call database.php. It basically select some stuff from mysql.. and in my index.php, i've include the class file and spit out all the data.

 

Below are the codes

 

database.php

<?php

class database{

private $mysql;

function __construct(){
	$this->mysql = new mysqli('localhost','root','','data');
}

//a function to select our stuff	
function select_query($sql){
	if(($result = $this->mysql->query($sql)) != NULL){	
		while($row = $result->fetch_object()){
			$content[] = $row->title;	
			$content[] .= $row->content.'<br />';
		}
	}else{
		echo $this->mysql->error;
	}
	return $content;
}

function __destruct(){
	$this->mysql->close;
}

}
?>

 

 

index.php

<?php

require 'database.php';

$database = new database();

$myData = $database->select_query('select * from contents order by id asc');


?>

<?php foreach($myData as $data): ?>
	<span class="list"><?php echo $data;?></span><br />
<?php endforeach;?>

 

Any advice is welcome. Thanks!

[Q]

A quick read and it seems fine to me

 

I only have one point I'd like to make, and it's nothing about your OOP; but you should really consider writing your MySQL functions in capital letters. Makes it so much easier to read.

 

The SQL you have there would be written like this:

SELECT * FROM contents ORDER BY id ASC

[irkevin]

Hey good timing. lol.. I don't even know why I wrote it in lowercase. I always do it the uppercase way.. Anyways I have one more question.

 

Is it ok to echo out stuff directly from the class file?

 

Lets say i want to echo out the number of rows I have. I would I proceed with that?

[Q]

I'd return it as a result instead, so that you just echo your function call where you need your numbers of rows.

 

I think this is the best practice for OOP too

[irkevin]

Hmm i see.

 

But now the only thing that bugs me is

 

When i get the result.. they come like this

 

Title number 2

Content Number 2

 

Title number 1

Content number 1

 

In the page source code, it's like this:

span class="list">Title number 2 </span><br />
<span class="list">Content Number 2<br /></span><br />
<span class="list">Title number 1</span><br />
<span class="list">Content number 1<br /></span><br />

 

As you can see, ebery span has the same class, since i'm echoing the variable $data in my foreach. Is there anyway I can echo out the title with it's own span and the content with it's own span?

[Q]

Can you try making a print_r() of your array?

 

<pre>
<?php
print_r($myData);
?>
</pre>

[irkevin]

Array
(
    [0] => Title number 2 
    [1] => Content Number 2<br />
    [2] => Title number 1
    [3] => Content number 1<br />
)

 

This is what I get

[irkevin]

Hmm nothing?

[KevinM1]

A couple of issues regarding theory:

 

1. Your class is unnecessary because MySQLi is already a class.  You're merely adding a wrapper to it.

 

2. That wrapper is defined too narrowly to be universally useful.  You're assuming that the only data you'll ever need to obtain are from columns named 'title' and 'content.'  What if the database doesn't have such columns?  You shouldn't need to change the internals of a class to make it useful.

[irkevin]

This is just a basic test since i'm still learning oop.. 

[KevinM1]

This is just a basic test since i'm still learning oop.. 

 

Still a good thing to think about, IMO.  A lot of times people think they're writing OOP code when in actuality they're either putting a wrapper around an existing class, without doing so to extend or otherwise merge that class into the greater system, or they're writing a basic function library with object/class syntax.  Learning and playing with the syntax is only part of the battle.

 

I'm not saying that you won't, or don't have the desire, to learn the theory.  I'm just saying that as you get comfortable with the nuts and bolts of writing syntactically correct code, you should be asking questions like "Does this class attempt to do too much?  Is it too narrowly defined?  What do I gain by doing things this particular way?  Is it sufficiently encapsulated?  Or will I have problems trying to add it to another system?"  It'll save you headaches down the road, believe me.

[irkevin]

I think you're right. I was just googling to see a better way of writing oop and and someone did the same thing as i did, needless to say, he got the same comment. lol.

 

Anyways, do you have a good place to start learning oop?

[trq]

Theres a few pretty good tutorials in this very site.

[ignace]

OO is much more then just fancy classes, it's meant to improve re-use. Their are many techniques that you can apply that will improve the re-use of your code, one of these techniques is loose coupling. When writing classes you should make sure your class follows the Single Responsibility Principle (SRP, http://en.wikipedia.org/wiki/Single_responsibility_principle) and always remember to refactor your code when necessary (http://en.wikipedia.org/wiki/Code_refactoring) also don't repeat yourself (DRY, http://en.wikipedia.org/wiki/Don't_repeat_yourself) and keep it simple (KISS, http://en.wikipedia.org/wiki/KISS_principle)

 

Is it ok to echo out stuff directly from the class file?

 

No it is not. Same applies for formatting directly in html (unless the classes's single responsibility is the formatting of data to html). Always make sure their is a clear separation of concerns (http://en.wikipedia.org/wiki/Separation_of_concerns) between your classes.

 

When you are writing code always remember to stick to some identifier naming convention. It makes your code more readable and maintainable.

 

Now to come back to your code and your actual question: when you write a class you need to think about the added value that this class makes to your application. For example you have created a Database class, however it has no added value. All it does is proxy MySQLi, actually your class is making it even worse as it limits the possible result set MySQLi provides. If I perform Database::select_query('..'); all I'll ever get is a numerical array? And what's worse of all, all my future tables needs to have a title and a content column. And if it has no added value then don't write it and use what's available.

[ignace]

$content[] = $row->title;
$content[] .= $row->content.'<br />';

 

is incorrect, what you actually should do is this (assuming you know what I mean with $i):

$content[$i]  = $row->title;
$content[$i] .= $row->content.'<br />';

[trq]

Or simply...

 

$content[] = $row->title.$row->content.'<br />';

[ignace]

Or simply...

 

$content[] = $row->title.$row->content.'<br />';

 

Even better yet