Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am working on converting a PHP application to MVC, and have a couple questions!

1) I have a main Model object, that I require a database connection for, as well as a User object that uses a database object also. Is it a bad practice to create a new database object for each of these classes, like the following?

or should I create a $databaseObject variable on the index.php page and pass that same variable in as a construct parameter for both User and Model? What is the difference?

Is there anything else that sticks out as something that I am doing wrong?

class Model {

    function __construct() {        
        $this->user = new User();
        $this->db = new Database( DB_TYPE, DB_HOST, DB_NAME, DB_USER, DB_PASS );
    }
}


class User {

    public function getUserId(){        
        return $this->_userid;  
    }

    public function __construct( ){
        $this->db = new Database( DB_TYPE, DB_HOST, DB_NAME, DB_USER, DB_PASS );
        $this->_userid = 1;     
    }
}

Thanks in advance for the help!

share|improve this question
3  
Yes, this is WET code, bad code. Also consider: How Not To Kill Your Testability Using Statics –  deceze Feb 15 '13 at 20:52
2  
Model is a layer, that contains multitudes of different instances from different types, not a class or object. Also, you code completely ignores SOLID principles. Is completely untestable and relays on global state for configuration. –  tereško Feb 15 '13 at 21:05
    
so I should pass in the database object into the constructor? –  user1582882 Feb 15 '13 at 21:15
2  
@user1582882 Yes, it's called dependency injection. Also, pass in the userid to the User object. –  crush Feb 15 '13 at 21:40
add comment

migrated from stackoverflow.com Feb 16 '13 at 17:13

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

1 Answer

In general, using the "new" keyword in any class is bad practice, because you're creating an implicit dependency. Instead, you should follow the "ask, don't look" design philosophy and require that the database object be passed to the class, rather than letting the class fetch it. Dependency injection is a common solve to this problem, and I'd recommend looking at how PIMPLE is used. Here's a good article: "Dependency Injection with Pimple".

Many people, like deceze, will deride you (rightfully so) for doing things like this, because it's one of the easiest (and probably most common) things you can do to kill the possibility of testing your code. If you were to write a unit test for this model, you'd have a database object being constructed with each test. Since the whole point of unit tests is to run code in isolation (to verify the tested code's accuracy), it would be impossible to test this model without a database.

On that note, the JavaScript guru, Misko Hevery, published a really superb article on writing testable code: "Top 10 things which make your code hard to test". While this is specifically in the context of code testing, it's also very good general programming information.

share|improve this answer
add comment

Your Answer

 
discard

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