5
\$\begingroup\$

Just looking for some feedback for one of my models I have for a project. I'm pretty happy with it but I was wondering if anyone had any optimization suggestions.

class ContactModel
{
    public function __construct($db)
    {
        try {
            $this->db = $db;
        } catch (PDOException $e) {
            exit('Database connection could not be established.');
        }
    }

    public function getBlastRecipients()
    {
        $sql = "SELECT Email FROM Contact WHERE Blast=?";
        $query = $this->db->prepare($sql);
        $query->execute(array('1'));
        return $query->fetchAll();
    }

    public function getBOLRecipients($custpoid)
    {
        $sql = "SELECT [CustPOID]
                          ,[POShipToRef]
                          ,cp.CarrId
                          ,cp.LoadID
                          ,c.CarrDesc
                          ,ct.Email
                          ,ct.ContactId
                      FROM [Sales].[dbo].[CustPO] as cp
                      LEFT OUTER JOIN Load as ld
                      on ld.LoadID = cp.LoadID
                      LEFT OUTER JOIN Carrier as c
                      on c.CarrId = cp.CarrId
                      LEFT OUTER JOIN Contact as ct
                      on c.CarrId = ct.CarrierId
                       WHERE CustPOID=?";
        $query = $this->db->prepare($sql);
        $query->execute(array($custpoid));
        return $query->fetchAll();
    }

    public function getDeliveryRequestEmailAddresses($custpoid)
    {
        //If a ShipToID is set, we want to return all email addresses associated with that customer, otherwise just return whatever email addresses are associated with the BillToID
        $sql = "SELECT
                       CASE
                        WHEN cp.CustShipToID IS NOT NULL THEN
                       c.Email
                       ELSE
                       cy.Email
                       END as Email
                         FROM CustPO cp
                         LEFT OUTER JOIN Contact as c
                         on cp.CustBillToID = c.CustId
                         LEFT OUTER JOIN Contact as cy
                         on cp.CustShipToID = c.CustId
                         WHERE CustPOID=?";
        $query = $this->db->prepare($sql);
        $query->execute(array($custpoid));
        return $query->fetchAll();
    }

    public function getEmailAddress($contact_id)
    {
        $sql = "Select Email FROM Contact WHERE ContactId=?";
        $query = $this->db->prepare($sql);
        $query->execute(array($contact_id));
        return $query->fetch();
    }
}
\$\endgroup\$
5
\$\begingroup\$

A bit of background, with some suggestions (in addition to the points tim mentioned):

  • In the MVC pattern, the M (Model) isn't really something that can be poured into a single class. The Model part (with an upper-case M) is referred to as a Layer. If you write a data provider (a class that queries the DB), it's part of the Model layer. If you write a class that does computation or data processing (a service), it is part of the Model layer. If you write a class that simply contains data that belongs together (ie: an Id, the name and email of a user), this class is called a data model (with lower case m), and it, too, belongs in the Model layer.
  • With this in mind, I'm afraid I have to say your class (ContactModel) has a rather unfortunate name. I'd expect a ContactModel to be a data model. A container that uses getters and setters (which sanitize and validate the data), which I can pass around. I'd expect a CustomerDataProvider class or CustomerMapper class to be present to take care of the queries. The SELECT queries here should then return one or more CustomerModel instances
  • Also important in the MVC pattern is to keep your controllers small. All a controller does, really, is to take the request parameters (user input), process it (pouring it into data models), and pass it on to the Model layer. This layer contains all of the actual logic. That's why it is also referred to as the business logic layer. This is the backbone of your application, it's where the magic happens.

So in basically, the traditional flow of an MVC application looks as follows:

[User Request]
  ||
  |_==> Dispatcher (create Request object, figure out routing)
         ||
         ||<== dispatcher works out what controller and action to use
         ||
         |_==> Controller extracts data from Request (creating data models)
              /\ ||
             //  ||<== works out what part of the Model layer processes data
             ||  ||
     returns ||  |===> [SERICVE] This is where it all happens
             ||         ||
             |_=========_| <-- after service is done

What the service returns to the controller can then be passed on to another component of the Model layer (in case further computation is required), or the controller passes it on to the view.

Now the service layer is where all things important live. Suppose we're processing a login form. In that case, the controller action should look something like this:

public funciton loginAction(Request $req)
{
    $form = new Form('loginFrm');//create the form
    if ($req->isPost())
    {//form was submitted
        $form->processRequest($req);//the form processes the request
        if ($form->isValid())
        {//form submission was valid, meaning all fields were filled in
            $service = new LoginService($config);//pass dependencies if applies
            $login = $service->doLogin($form);
            if ($login)
            {//do the actual login in service
                return $this->redirect('userMainPage', $login);
            }
            $form->setError('Invalid login');//set error message
        }
        else
        {//Form was invalid
            $form->setError('Not all required fields were filled');
        }
    }
    //pass form to view, ready to be rendered
    $this->renderView(
        'login.twig',
        [
            'form' => $form
        ]
    );
}

Take away the comments, and my rather spacious coding style, and you'll find that all of the code in my controller could be condensed to about 10 lines, without having to sacrifice readability too much. That's how it should be. However, let's look at this one line a bit more closely: $service->doLogin($form).
Like I said many times before already: everything that relies on external data (webservice calls, DB connectivity), or requires more complex computation does not belong in the controller. Hence, this simple looking service call will contain the most code.

About now, it might be good to mention the SOLID principles. More importantly the S part: Single Responsibility Principle. This very reasonable principle states (or rather dictates) that a class can have only one reason to change. That's a fancy way of saying that a class can have no more than one job. A class focusses on just one task: a data model's job is to contain data. That's all that it does. It can't connect to the DB, nor can it query for data. That's not its job. A DataProvider queries a database, but it doesn't directly connect to it: It might contain a PDO instance, which represents the connection, or it can be injected the config to establish this connection, but the connecting itself is done my PDO or mysqli. Just like these two PHP extensions don't occupy themselves with the actual result fetching (they return PDOStatement and mysqli_stmt instances to do that).
At this point, we've already mentioned 5 elements that make out our Model layer:

  • Data models
  • Service
  • DataProvider
  • DB connection (PDO or mysqli)
  • DB resultsets and queries (PDOStatement, or mysqli_stmt)

Each of these components have a clear task to perform, and logic dictates that each task can go wrong. Therefore, each of these components must have means to notify the user that they were unable to do their job. If one of these components simply calls exit or die, you'd have the devil's own job of figuring out what component it is that fails, and why. Enter Exceptions. Let's examine what can go wrong, and how we can easily notify the user of this issue:

  • Data models: if, for example, a ContactModel is passed an invalid email address to its setEmail method, the model should pick up on this (using filter_var($argument, FILTER_VALIDATE_EMAIL)). If the email is invalid, the model should throw an InvalidArgumentException with a message like '"foo" is not a valid email address'
  • The Service binds everything together, so it depends on there being a data provider, valid data and any number of other dependencies available. If one of them is missing, a service might throw a RuntimeException instance, telling you (the user) it's missing a data provider it needs
  • The DataProvider in turn requires a db connection to execute queries, if this is missing, it should throw another RuntimeException. If you pass an instance of ContactModel to a method called findContactByEmail, but this instance doesn't have an email set, the dataprovider could throw a LogicException or a BadMethodCallException.
  • The DB connections and statement/resultset objects will behave as you want, but it's common to have PDO throw PDOException instances.
    mysqli is a bit trickier. It's api is somewhat messy, so it's very common to see it wrapped in a class that checks for errors and throws a mysqli_sql_exception or mysqli_warning if something has gone wrong

So depending on what goes wrong, in what component, a different exception will be thrown. That makes your life a lot easier, because if you see a PDOException flying by, you simply know where to look.

So now, what does this LoginService::doLogin method look like? Well, this would be plausible:

/**
 * Validates login form, returns full user model on success, false on failure
 * @param Form $data
 * @return UserModel|bool
 * @throws RuntimeException
 */
public function doLogin(Form $data)
{
    $user = new UserModel();
    $user->setUserName($data->getLogin())
        ->setPassword($data->getPassword());
    $provider = $this->getUserDataProvider();
    if (!$provider)
    {
        throw new RuntimeException(
            sprintf(
                '%s did not receive the user dataprovider',
                __CLASS__
            )
        );
    }
    if (!$provider->userExists($user))
        return false;//user not found
    $existing = $provider->getUserByLogin($user);//get full data
    if ($existing->getHash() === $user->getHash())
        return $existing;//return full data model
    return false;
}
\$\endgroup\$
7
  • \$\begingroup\$ Excellent explanation, one comment: Some text and developers sometime refers the "service" layer as a Repository (Jeffrey Way from Laracast does so). \$\endgroup\$ – azngunit81 Dec 31 '14 at 19:44
  • \$\begingroup\$ @azngunit81: To me, a repository sounds like a DBAL. I agree that DB interaction has no place in a controller, it's handled by a repository manager (or a simple mapper, or some other mechanism), which is accessed in the service layer. But to me, a service can be a lot more than a repository. Services can, for example, be used to handle API calls, file IO, logging, or whatever. Possibly, we're both talking about the same thing but I found this when googling for Jeffrey Way \$\endgroup\$ – Elias Van Ootegem Jan 2 '15 at 17:46
  • \$\begingroup\$ Where it says "I’m going to use Repositories to abstract the database layer away.", which uses the term "repository" in the same way as a Doctrine repo. To me, the service layer is much broader than just that \$\endgroup\$ – Elias Van Ootegem Jan 2 '15 at 17:46
  • \$\begingroup\$ That reference is from another author that breaks down Laravel usage specifically. However in Jeffrey's Way earlier laracast - he specifically shows you how he restructure his SRC folder to remove "models" and use repositories and providers instead. \$\endgroup\$ – azngunit81 Jan 2 '15 at 21:00
  • \$\begingroup\$ @azngunit81: Providers and repos are, once again, terms that hint at a DBAL, they are indeed part of the service layer, but they're not the only thing in there \$\endgroup\$ – Elias Van Ootegem Jan 2 '15 at 21:21
5
\$\begingroup\$

I just have a couple of small points:

Exiting in Constructor

You shouldn't just exit in a constructor, but throw an exception instead. But in this case, the only statement in the try block is an assignment, so there isn't really anything that can go wrong. You should just remove the entire try-catch block.

Naming

Your naming is somewhat inconsistent. For methods you use camelCase, and for parameters you either use all lowercase (which you should always avoid), or under_score. At the very least custpoid should be custpoId.

I would also use somewhat more descriptive names inside the queries. Having c, cp, cy, as well as ct all in one query is a bit confusing.

SQL Style

Your SQL statements are also a bit inconsistent. If you write SQL keywords in all uppercase, do it always (see for example as, on, and Select).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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