Separating PHP code and logic

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(

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))

StackExchange Code Review Q#160281, answer score: 2