2
\$\begingroup\$

I have only recently found out about 'Data Mappers' which allows data to be passed into classes without said classes knowing where the data comes from, so here is my attempt to write one:

index.php

include 'classes/db.php';
include 'classes/usermapper.php';
include 'classes/user.php';

$user = new User;
$userMapper = new userMapper;

try {
  $user->setData([
    $userMapper->fetchData([
      'username'=>'peter1'
    ])
  ]);
} catch (Exception $e) {
  die('Error occurred');
}

if ($user->hasData()) {
  echo $user->fullName();
}

classes/user.php

class User {
  private $_data;

  public function __construct() { }

  public function setData($userObject = null) {
    if (!$userObject) { throw new InvalidArgumentException('No Data Set'); }
    $this->_data = $dataObject;
  }

  public function hasData() {
    return (!$this->_data) ? false : true;
  }

  public function fullName() {
    return ucwords($this->_data->firstname.' '.$this->_data->lastname);
  }
}

classes/usermapper.php

class userMapper {
  private $_db;

  public function __construct() { $this->_db = DB::getInstance(); }

  public function fetchData($where = null) {
    if (!is_array($where)) { 
      throw new InvalidArgumentException('Invalid Params Supplied'); 
    }

    $toFill = null;
    $toSanitize = [];

    foreach($where as $argument=>$value) {
      $toFill .= $argument.' = ? AND ';
      array_push($toSanitize, $value);
    }

    $query = sprintf("SELECT * FROM `users` WHERE %s ", substr(rtrim($toFill), 0, -3));


    $result = $this->_db->queryn($query, $toSanitize);

    return $result;
  }
}

classes/db.php

class DB {
  private static $_instance = null;
  private $_pdo,
          $_query,
          $_error = false,
          $_results,
          $_count = 0;

  private function __construct() {
    try {
      $this->_pdo = new PDO("mysql:host=localhost;dbname=dbname", "username", "password");
    } catch (PDOException $e) {
      die('DATABASE ERROR. SOMETHING HAS GONE WRONG.');
    }
  }

  public static function getInstance() {
    if (!isset(self::$_instance)) {
      self::$_instance = new DB();
    }
    return self::$_instance;
  }

  public function queryn($query = null, $pA = array(), $esp = false, $enr = false, $tdb = false, $rid = false) {
    if (!$query) {
      throw new InvalidArgumentException('Misconfigured Parameters. Need $query, $pA [Array], $esp [true/false], $enr [true/false], $tdb [true/false]');
    }

    /* FILL IN BLANKS */
    if (!$esp) {
      $esp = false;
    }
    if (!$enr) {
      $enr = false;
    }
    if (!$pA) {
      $pA = [];
    }
    if (!$tdb) {
      $tdb = false;
    }

    /* PREPARE QUERY AND BIND PARAMS */
    if ($this->_query = $this->_pdo->prepare($query)) {
      for ($i=0; $i < count($pA); $i++) {
      if (is_string($pA[$i])) {
            $type = PDO::PARAM_STR;
        } else if (is_int($pA[$i])) {
            $type = PDO::PARAM_INT;
        } else if (is_bool($pA[$i])) {
            $type = PDO::PARAM_BOOL;
        } else if (is_float($pA[$i])) {
            $type = PDO::PARAM_STR;
        } else {
            $type = PDO::PARAM_NULL;
        }
        $this->_query->bindValue($i+1, $pA[$i], $type);
      }

      /* CHECK EXECUTION SUCCESS */
      if (!$this->_query->execute()) {
        throw new Exception('Query Failed to Execute');
      }

      /* SET ROW COUNT */
      $this->_count = $this->_query->rowCount();

      /* FOR EXPECTING NO RESULTS */
      if ($enr === true) {
        if ($this->_query->rowCount() == 0) {
          return true;
        } else {
          return false;
        }
      }

      if ($tdb === true) {
        if ($this->_query->rowCount() == 0) {
          throw new Exception('No DB Item Changed.');
        }
      }

      /* RETURNS NULL WITH NO RETURN RESULTS */
      if ($this->_query->rowCount() == 0) {
        return null;
      }

      /* RETURN DATA BASED ON ESR [ExpectSingleResult] */
      if ($esp === false) {
        $this->_results = $this->_query->fetchAll(PDO::FETCH_OBJ);
      } else {
        $this->_results = $this->_query->fetch(PDO::FETCH_OBJ);
      }

      /* Return Results */
      if ($rid === true) {
        return ((int)$this->_pdo->lastInsertId() === 0) ? null : $this->_pdo->lastInsertId();
      } else {
        return $this->_results;
      }


    } else {
      throw new Exception('Failed to Prepare Query.');
    }
  }

I am asking whether this code could be improved in general, and I am interested in particular whether it could be optimized. That way, I won't have to create a distinct mapper class for the every entity, as they all seem identical to me, making a lot of duplicated code.

\$\endgroup\$
11
  • \$\begingroup\$ $result = $this->_db->query($query); //assume this is just a call to a database which returns the results of the query Did you simplify your code before posting it here? Please don't. \$\endgroup\$
    – Mast
    Mar 1, 2018 at 19:18
  • \$\begingroup\$ Welcome to Code Review! I'm afraid this question does not match what this site is about. Code Review is about improving existing, working code. The example code that you have posted is not reviewable in this form because it leaves us guessing at your intentions. Unlike Stack Overflow, Code Review needs to look at concrete code in a real context. Please see Why is hypothetical example code off-topic for CR? \$\endgroup\$ Mar 1, 2018 at 21:04
  • \$\begingroup\$ Hi @Dannnno. Thank you for your comment. I appreciate your reply but would like to point out the code above is fully understandable and complete. The intentions are outlined above the code block 'Let's assume we have a simple application whos purpose is to echo out a first name and last name of a user.'. I also am looking for someone to hopefully point me in the right direction on how to proceed with DataMappers in the domain mapper model context, hence a review of my code. If this still breaches the guidelines, i will be more than happy to remove my question. Thank you for your understanding. \$\endgroup\$
    – Matthew M
    Mar 1, 2018 at 21:09
  • \$\begingroup\$ You're currently 3/5 close votes into having your question closed. ultimately, the problem is that you're asking for guidance as to best practices with what seems like example code, not a real problem and solution. What might make sense in one situation might not in another, and without that context there isn't a whole lot of useful review we can give. Additionally, us reviewing example code isn't nearly as useful to you and wastes our time. \$\endgroup\$ Mar 1, 2018 at 21:14
  • 1
    \$\begingroup\$ What is interesting, none of the voters seem have any experience with PHP and I wonder what is their business about this question at all. Closing it under the present reasons is a stretch. \$\endgroup\$ Mar 2, 2018 at 7:52

1 Answer 1

2
\$\begingroup\$

What you want can be implemented using inheritance.

The base entity

You can define a base mapper class where all the common stuff would be defined:

abstract class defaultMapper
{
    protected $_db;
    protected $_table;
    protected $_entity;

    public function __construct(MyPDO $pdo) {
        $this->_db = $pdo;
    }

    public function find($id) {
        $sql = "SELECT * FROM `{$this->_table}` WHERE id=?";
        return $this->_db->run($sql, [$id])->fetchObject($this->_entity);
    }
}

And then only extend it when creating particular mappers, defining only specific properties such as table or entity name:

class userMapper extends defaultMapper
{
    protected $_table = 'users';
    protected $_entity = 'User';
}

so in the end it would look like this

$userMapper = new userMapper($db);
$user = $userMapper->find(104);

The database class

Note that your DB class is heavily bloated and has numerous issues. The case is so common than I even wrote a dedicated article about common mistakes in database wrappers. So in the code above I used a concise example from my other article. Sending the DB instance in the constructor is considered the best practice though if it's hard for you, then you can use the static variant shown there as well.

The magic mapper

In theory, you could even avoid creating particular mappers for your entities, by means of using some considerations, like a table name is always a lowercased entity name, the unique identifier field is always called 'id' and such. You can find an example implementation in my earlier question, as well as a very productive criticizm in the answer there below.

You may also look at the Unit Of Work concept which is using dynamically created mappers or managers: http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/unitofwork.html

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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