Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I'm converting a procedural based site to an OOP design to allow more easily manageable code in the future and so far have created the following structure:

/classes
/templates
index.php

With these classes:

ConnectDB
Games
System
User
User   -Moderator
User   -Administrator

In the index.php file I have code that detects if any $_GET values are posted to determine on which page content to build (it's early so there's only one example and no default):

function __autoload($className) {
    require "classes/".strtolower($className).".class.php";
}

$db = new Connect;
$db->connect();

$user = new User();

if(isset($_GET['gameId']))
{       
    System::buildGame($gameId);
}

This then runs the BuildGame function in the system class which looks like the following and then uses gets in the Game Class to return values, such as $game->getTitle() in the template file template/play.php:

function buildGame($gameId){

    $game = new Game($gameId);
    $game->setRatio(900, 600);

    require 'templates/play.php';
}

I also have .htaccess so that actual game page url works instead of passing the parameters to index.php

Are there any major errors of how I'm setting this up or do I have the general idea of OOP correct?

share|improve this question

migrated from stackoverflow.com Jan 3 '12 at 14:28

This question came from our site for professional and enthusiast programmers.

    
I can't see major nor concept errors. You are doing it well. But let's wait for more expert opinions :) –  lorenzo-s Jan 3 '12 at 14:15
    
@Silver89 -- can you put your code up somewhere? (github or the like?) –  Neal Jan 3 '12 at 14:17
    
whether you got the OO parts right or not is impossible to say from the very little code you show. We'd need to see your classes. If you need guidance in refactoring the procedural codebase have a look at sourcemaking.com/refactoring/… –  Gordon Jan 3 '12 at 14:21
1  
There isn't enough info in your question, to discuss your approach. You should expand it and give us a better view of the whole hierarchy (not so much code, but how your classes interact with each other) and tell us exactly what you're doing and why. Or you could simply post your classes for review at Code Review Stack Exchange. –  Yannis Rizos Jan 3 '12 at 19:58
    
You might want to consider implementing an autoloader instead of explicitly requiring everything. –  GordonM Jan 4 '12 at 10:12
show 2 more comments

3 Answers 3

up vote 9 down vote accepted

As far as OOP goes, there's never really a right or wrong way. There's always more roads that lead to Rome.

However, there are some caveats to avoid. Your System class looks like a God object, which is a so-called 'anti-pattern'. Best to avoid those. Also, I suspect that you're using $db and $user as global variables -- those are not encouraged in OOP. It's best to pass the DB handle to the classes that need it, either upon construction or for instance with a setDatabase() method (this is called Dependency Injection, and makes your classes easier to oversee, maintain and debug).

Finally, an autoloader might make your job as a programmer a little bit easier. This will take the job of doing the require calls out of your hands -- and also forces you to structure your code in a logical manner :)

share|improve this answer
    
All speculation. I find it pretty hard to judge that System is a God Object. It might just be an Application Factory. Same for the remaining answer parts. You simply cannot know from the OP's code. –  Gordon Jan 3 '12 at 14:24
1  
+1 for the autoloader. –  Raveline Jan 3 '12 at 14:24
    
+1 for explaining Dependency Injection –  Mark Baker Jan 3 '12 at 17:58
add comment

You might take a look at an MVC... you're already splitting your templates and classes for the View and Model/Controller) components, but look at splitting your classes into models and controllers

share|improve this answer
    
Yes I think this is what I'm trying to aim for, quite a few of these terms are quite new to me though so I'm still learning how to go about implementing these solutions. I've just got to the point where I'm beginning to see the real benefits of OOP. –  Silver89 Jan 3 '12 at 14:20
add comment

You might want to employ an autoloader to load all your classes for you.

You could see how I tried to do it in my (m)vc.

share|improve this answer
    
yes, but the OP asked about OOP and I dont see how this suggestion is on-topic. Should have been a comment. –  Gordon Jan 3 '12 at 14:18
    
@Gordon might help the OP organize their code better. –  Neal Jan 3 '12 at 14:19
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.