Separating PHP code and logic

Question

I am new to OOP and slowly starting to learn it to increase my PHP. This is my first attempt at writing something using OOP.

Now people always talk about separating the logic and php code from the HTML / views. This is a rather difficult concept for me to grasp thus, I tried making my own little MVC "framework" simply to fool around in an attempt to learn new concepts. I came up with the following.

CONTROLLER

require_once("../config/db.php");

class Employer
{
    public $name, $location, $email;
    private $password;
   
     function setPword($newPword){
    $this->password = $newPword;
    }

    function getPword(){
    return $this->password;
    }

    function setEmployerName($newName)
    {
        $this->name = $newName;
    }

    function getEmployerName()
    {
        return $this->name;
    }

    function getEmployerFirstname(){
        $fullname = $this->getEmployerName();
        $fullname = explode(' ', $fullname);
        return $firstname = $fullname[0];
    }

    function getEmployerLastName(){
        $fullname = $this->getEmployerName();
        $fullname = explode(' ', $fullname);
        return $lastname = $fullname[1];
    }
    
    function  setEmail($newEmail){
        $this->email = $newEmail;
    }
    function getEmail(){
        return $this->email;
    }

    public function isRegistered(){
        global $db;
        $email = $this->getEmail();
        $sql="SELECT email FROM users WHERE email = :email";
        $stmnt = $db->prepare($sql);
        $stmnt->bindValue(":email", $email);
        $stmnt->execute();
        $stmnt->fetchAll();
        if($stmnt->rowCount() > 0){
            return die("Email Already Registered");
        }
            return '';
    }

    function registerNewEmployer(){
        global $db;
        try {
            $firstname = $this->getEmployerFirstname();
            $lastname = $this->getEmployerLastName();
            $email = $this->getEmail();
            $location = $this->getLocation();
            $pword = $this->getPword();

            $sql = "INSERT INTO users (firstname, lastname, email, pword, userType, joinDate )
                 VALUES
            (:firstname, :lastname, :email, :pword, :userType, :joinDate)";
            $stmnt = $db->prepare($sql);
            $stmnt->bindValue(':firstname', $firstname);
            $stmnt->bindValue(':lastname', $lastname);
            //$stmnt->bindValue(':usernane', $username);
            $stmnt->bindValue(':email', $email);
            $stmnt->bindValue(':pword', $pword);
            $stmnt->bindValue(':userType', '2');
            $stmnt->bindValue(':joinDate', Date('Y-m-d')');
        }
        catch(Exception $e){
                $e->getMessage("You could not be registered at this time!");
            }
            $stmnt->execute();
            if ($stmnt->rowCount() > 0) {
                return $success = "New Employer Successfully Registered";
            } else {
                return $error = "Registration Failed";
            }
    }

MODEL

 <?php
        $URL = $_POST['url'];
        $page = explode('/', $URL);

            if($page[3] == 'signup.php'){
            require_once('../controller/users.php');
            $obj = new employer();
            $obj->setEmployerName($_POST['fullname']);
            $obj->getEmployerFirstname();
            $obj->getEmployerLastName();
            $obj->setPword($_POST['pword']);
            $obj->setEmail($_POST['email']);
            echo $obj->registerNewEmployer();
            ?>

VIEW

<form role="form" id="registerForm" name="registerForm" method="post" action="../model/router.php">
                <div class="form-group">
                    <label for="fullname" class="col-2 col-form-label">Fullname</label>
                    <div class="col-4">
                         <input type="text" name="fullname" required="required" id="fullname" class="form-control" placeholder="Enter Fullname" />
                    </div><!--/col4-->
                 </div><!--/form group-->
                <div class="form-group">
                    <label for="email" class="col-2 col-form-label">Email</label>
                    <div class="col-4">
                        <input type="email" name="email" required id="email" class="form-control" placeholder="Enter Email Address" />
                    </div><!--/col4-->
                </div><!--/formgroup-->
                <div class="form-group">
                    <label for="pword" class="col-2 col-form-label">Password</label>
                    <div class="col-4">
                        <input type="password" name="pword" required="required" id="pword" class="form-control" placeholder="Enter Password" />
                    </div><!--/col4-->
                </div><!--/form group-->
                <div class="form-group">
                    <label for="pword2" class="col-2 col-form-label">Confirm Password</label>
                    <div class="col-4">
                        <input type="password" required id="pword2" name="pword2" class="form-control" placeholder="Confirm Password" />
                    </div><!--/col4-->
                </div><!--/formgroup-->
                <div class="form-group">
                    <input type="text" style="display: hidden" name="url" value="<?php echo $_SERVER['REQUEST_URI'] ?>" />
                    <button class="btn btn-info btn-size form-control" name="submitBtn" id="submitBtn">Join</button>
        </div><!--/col12-->
            </form>

The code above is used for a basic registration form. Now all this is working fine however I have a few questions,

1. The Model, called router.php works as follows. The user submits the form it gets the page, explode() it and if the page name matches the page name hardcoded inside the router function it will process the PHP code derived from controller.

   The problem with this is since the page name is hardcoded in the router model $page = explode('/', $URL) if($page[3] == 'signup.php'){} if the page name changes it will result in an error. How can I improve this?

2. Furthermore I would really appreciate if someone could have a look at my registerNewEmployer() method. Is this a semi-acceptable way to code a model in the OOP realm? How can above mentioned model be improved? (keeping in mind I am a beginner)

3. Any other methods you see a problem with that needs improvement?

4. You will notice I use global $db inside my methods why is this considered a bad practice / what are the draw backs of doing this?

Any other errors problems / recommendations / advice will be enormously appreciated.

Answer 1

What you are calling a "controller", is really more of a model, and your "model" looks more like a controller.

---

Did some of your "controller" code get cut out on copy/paste? It is not current valid code the way it is shown.

---

Are first and last names properties of the Employer or not? Make up your mind here. If you want to have such first/last name constructs, then they should exist throughout your whole data model all the way from the entry form to the database. Personally, I don't see great value in dividing names up in this manner, but if you have such a need, I would collect it like this to begin with rather then depending on potentially bad/arbitrary logic to make this distinction. What if the name is Lee Harvey Oswald? Do you really want Harvey to be the last name?

---

Typically, an is* method would be expected to return a boolean. Your isRegistered() method oddly either kills program execution with a message to standard out, or returns empty string. Consider making this true/false return and let calling code figure out what to do form there. Don't directly output in a class like this, let code that is up the call stack and better positioned to render the view have this responsibility. Especially don't die() here, as you have no chance to recover gracefully then. Also, wouldn't this be a legitimately expected use case your application could encounter? Why die?

---

You seem to have a habit of creating unnecessary temporary variables. These just clutter your code. Consider this section of code:

        $firstname = $this->getEmployerFirstname();
        $lastname = $this->getEmployerLastName();
        $email = $this->getEmail();
        $location = $this->getLocation();
        $pword = $this->getPword();

        $sql = "INSERT INTO users (firstname, lastname, email, pword, userType, joinDate )
             VALUES
        (:firstname, :lastname, :email, :pword, :userType, :joinDate)";
        $stmnt = $db->prepare($sql);
        $stmnt->bindValue(':firstname', $firstname);
        $stmnt->bindValue(':lastname', $lastname);
        //$stmnt->bindValue(':usernane', $username);
        $stmnt->bindValue(':email', $email);
        $stmnt->bindValue(':pword', $pword);

This could be condensed to:

        $sql = "INSERT INTO users (firstname, lastname, email, pword, userType, joinDate )
             VALUES
        (:firstname, :lastname, :email, :pword, :userType, :joinDate)";
        $stmnt = $db->prepare($sql);
        $stmnt->bindValue(':firstname', $this->getEmployerFirstname());
        $stmnt->bindValue(':lastname', $this->getEmployerLastName());
        $stmnt->bindValue(':email', $this->email);
        $stmnt->bindValue(':pword', $this->pword);

Also, you can use the object properties rather than getters form inside the class.

The same thing here:

function getEmployerFirstname(){
    $fullname = $this->getEmployerName();
    $fullname = explode(' ', $fullname);
    return $firstname = $fullname[0];
}

This could be

function getEmployerFirstname(){
    $fullname = explode(' ', $this->name);
    return $firstname = $fullname[0];
}

---

$e->getMessage("You could not be registered at this time!");

getMessage() is a getter, not a setter, and in fact takes no arguments.

You are also just swallowing your exception and would actually then attempt execute() against a failed statement on the next line. My suggestion is to not use try-catch if you are not going to do something meaningful in the catch block like gracefully recovering so calling code can continue operating or wrapping and re-throwing the exception to abstract implementation details from caller. If you are not doing something like this, you should just let the exception bubble up the stack until it gets to some code that is properly positioned to recover from the exception.

The main thing though is don't swallow exceptions.

---

Get in the habit of passing meaningful return results from model classes like your Employer class. The registration method for example should probably just return a boolean. It should not be this class' job to form end user messaging around success/failure like you are doing in your return strings.

---

Date('Y-m-d')

Is this a typo for date() or does your application have its own Date class?

---

I am generally worried about the Employer class as it is subject to be put in a bad state. Calling code could arbitrarily change certain properties and in essence insert multiple dirty copies of the employer records into the database with only slight variants. You need to make some decisions about how mutable this object should really be and what the call patterns to the database should be to update records.

This might be OK for your simple form-input use case, but what about when you want to re-use this class in other parts of your application?

I would consider changing your controller method to something like:

public function registerNewEmployer($name, $password, $email)) { ... }

That way you make the act of registering the employer atomic from the perspective of the caller. They could get a true/false response and know authoritatively whether this registration was successful.

---

You have no validation around your classes' public methods where parameters are passed, and therefore are allowing your object to potentially put into a bad state by their callers.

You make matters worse by passing user input from $_POST directly into the class methods. Why would you even bother making a database call if you have been given a malformed email address, or an empty string for a user name, or a one-letter password?

---

Stay away from using global declaration. This is really an archaic way of programming that fell out of favor 10 years ago, but still renders its ugly head in PHP codebases and code samples out there in the wild. If a class/method/function needs a database connection, pass it in as a dependency.

Why is this bad? What happens to your code if this variable does not even exist? What happens to your code is this variable is not set in a proper state? What happens if you, at some point in the future, want to change this variable name?

What you see is that by using global you heavily couple your code to this variable. If, however you pass it the dependency like this:

public function __contruct(PDO $pdo) {
    $this->pdo = $pdo;
}

You force the code that instantiates this class to give you the proper dependency, in a proper state you need for you code to work. This is known as dependency injection. This free you up from having to litter your class with global declarations as you know with 100% certainly that you have a valid dependency passed to your object (otherwise object instantiation would fail).

---

       $obj->getEmployerFirstname();
       $obj->getEmployerLastName();

In your "model" (which is really a controller), you call these lines of code unnecessarily, as these are called in your registration method anyway.

---

I am seeing from your form element that you have a URI structure like ../model/router.php. This seems really odd. Why would the endpoint be named router? Why would it be nested underneath model? You should look at strategies that common frameworks use to semantically name application endpoints. That would probably go a long way to clearing up your questions around routing.

For example, you might expect and endpoint like this to be the target endpoint for this form

POST http://yourdomain.com/employer/register

When inspecting this URI in a front controller's routing, you could easily break apart the URI to understand what controller should be used and what method is being called against that controller.

For example, a very rudimentary router/controller interaction might look like this:

// parse URI
$routeParts = explode('/', $_SERVER['REQUEST_URI']);

// instantiate base controller from first part of URI
// pass the rest of URI parts to controller so it can figure out what it needs to do
$controller = new {$routeParts[0]}($routeParts);
// execute the controller, passing application control to it
$controller->execute();

---

Stylistically:

• Stay away from trie shortening of variable/method names. For example, use getPassword instead of getPword.
• Your indentation is inconsistent, making your code harder to read.
• Be consistent in how you map getters/setters to property names. Why does get/setEmployerName get/set a property called name?
• Some of your method names are overly verbose. If you have a class called Employer and have a method to register a new employer, based on the values set in the properties, why not just call the method register()?

Answer 2

MVC

A simplified description of MVC responsibilities.

• Model: talks to the database and maintains data consistency.
• View: shows information to the user.
• Controller: takes input from the user.

Your "model" processes user input (the $_POST variables). Your "controller" talks to the database.

$db

Consider what happens if you write two pieces of software using the global $db. Now you want to combine them. How? It's two different databases. You end up rewriting one to remove the conflict.

This can get worse if you mix your software with someone else's. For example, in other languages $db might naturally represent something other than a database.