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've recently picked up OOP in Java and I'm trying to use it for my PHP usersystem.

I've come up with the following generic.class. On my main site, I'll simply put a include('generic.class.php'); before my content.

<?php
include_once('connect.class.php');

class User {

    private $id;
    private $username;
    private $email;

    public function __construct(){
        $this->id = $_SESSION['id'];
        $this->username = $_SESSION['username'];
        $this->email = $_SESSION['email'];
    }

}

class Generic {

    private $maintenance = false;

    public function __construct() {     
        // Start session
        session_start();

        // Check maintenance status
        if($this->maintenance){
            include('maintenance.php');
            die();
        }

        if(user_logged_in()){
            $user = new User();
        }
    }
}

function user_logged_in(){
    if (isset($_SESSION['username'], $_SESSION['id'])){
        return true;
    } else {
        return false;
    }
}

$generic = new Generic();
$conn = new Conn();
?>

Echoing $user->username doesn't seem to work. Or is there no need to use a User class at all? (most PHP usersystems I see don't have a User class)

Is this the right approach to using OOP and classes with a usersystem? If not, how should my classes be like?

share|improve this question

2 Answers 2

Couple of points:

  • Utilize Dependency Injection for your constructor to loosely couple it's dependencies.
  • In OOP, functions are not used that often, but methods. user_logged_in() should reside in some sort of User authentication handler (Single Responsibility Principle). Where you could do something like $usrAuthService->isLoggedIn().
  • Give your classes meaningful names. Generic and Conn are really bad names. What is a "Generic" class? What is "Conn"? Connection? Connection of what? In a month when you come back to visit your code, you will not understand at a glance what is happening.
  • A constructor's primary purpose is to initialize the object.

$user->username doesn't work because your class properties are private.

You must realize that there are many different design patterns out there that programmers may adapt for their systems. User systems that have entity classes like User are more domain oriented (DDD).

share|improve this answer

This is not good OOP.

The User constructor takes no arguments. It's basically creating a user from thin air. It would be better to make its parameters explicit. To make it convenient to create a user without parameters, it would be better to use a factory method with a good name, for example UserFactory.createFromSession. With this method it's still easy to use, and quite clear where the parameters will come from.

The Generic class is poor in many ways:

  • It has a practically meaningless name. A class should have a good name that explains well its purpose. When it's not clear how to name a class, it indicates a problem with the design.
  • The purpose of a constructor is to create an object. This constructor does something else. In fact it's just a utility method that shouldn't be in a class at all.

user_logged_in is not a great name. An in any case, it would be good to move this into a private method of UserFactory, and name it is_logged_in (the "user" is implied by the class). The factory method could return two kinds of users:

  1. Regular user, logged in, authenticated
  2. Anonymous user, not logged in

You could have different classes to represent these two types, sharing the same interface, and having a method like is_authenticated_user to differentiate functionality in different parts of your website. It's very practical and ergonomic to represent non-authenticated users with a non-null object this way. Many web frameworks use this technique.

share|improve this answer

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.