代码审查
Marcin Nabialek 已经回答了您的问题。在 foreach 中的那个突破是。好。它在那里做什么?
但是,您的代码中还有很多问题。所以这里是一个代码审查:
构造函数
你显然知道什么是构造函数。你在两个类中都使用它。但是,不一样。为什么?你的User 类有一个public function User(),但你的UserService 有一个public function __construct()。选择一个,并坚持下去。如果可以选择,请选择正确的:__construct()
来自phpdoc:
从 PHP 5.3.3 开始,与命名空间类名称的最后一个元素同名的方法将不再被视为构造函数。此更改不会影响非命名空间类。
所以命名你的用户类会破坏你的构造函数。现在这可能不是问题,但它闻起来很香。只需使用 __construct()。这是首选且正确的方法。我们生活在今天,而不是过去的 php4-days :)
代码样式
天哪,今天死了很多小猫!
有时你在新行上有一个括号:
if (isset($this->userdata[$var]))
{
return $this->userdata[$var];
}
有时你不会
if($this->userdata){
$this->userdata;
}
再次,选择一些东西并坚持下去。如果你想拯救一些小猫。坚持标准:PSR-1 & PSR-2
公共属性,jeuk
您的 User 类有一个公共变量 $attributes。所以它可以从外部世界访问。但是您也给我们提供了get 和set 方法。为什么?
一个好的规则是:public $var 有气味,protected $var 应谨慎使用,private $var 是好东西。
你的课是什么?为什么不使用 __constructors?
如果我查看 User 类,我总是查看 __constructor。 User 类不需要变量。所以我应该能够做这样的事情:
$me = new User();
$me->getName(); //who am I ?
这当然行不通。没有名字的用户没有意义。它总是有一个。那就去求吧!
class User
{
public function __construct($name)
{
$this->name = $name;
}
}
$me = new User('jeroen');
$m->getName(); //I am jeroen :)
如果您需要某些东西,请索取;)
所以:
public function __construct(Database $db)
是滚动的方式!
别让我读数据库
现在,您的 get/set 方法与您的数据库紧密耦合。如果更改数据库中列的名称。您可以将整个代码重构为 get('new_column_name'); 。听起来很有趣!
另外,这个方法对我说了什么?没什么,写起来容易吗?没有
getName 说出它的作用,它得到了我的名字。
get 告诉我我得到了一些东西。但是什么?
提出其他问题:get('name') =?= get('Name')
用户对象知道它有什么是可以的。
总结
好的,我在您的代码中概述了一些错误。您应该研究的一些概念:
- 固体
- PSR 标准
- 工厂模式(这将有助于您的 UserService
- 控制反转
因此,为了这篇文章,这里是您的代码修改。请注意,我直接将它写到了这个评论框中,所以我可能会遗漏一些东西并犯一些错误。
更改列表:
- 我清理了样式
- 我添加了一些 cmets
- 删除了 _destruct()(它什么也没做)
- 使用 PDO 而不是 Databse 类(不知道您在使用什么,但看起来像 PDO 包装器)
- 更改了一些表名和选择查询(切勿在 selct 查询中使用 *。Onyl 要求您提供所需的内容)
- 使用准备好的语句
- 更灵活
- 异常而不是返回null;
- 可以将您的 $DBH 放入单例/工厂以简化您的 $dbh 创建:Database::getInstance();或 DatabaseFactory::getInstance()->createPDO();或者。好久没写这样的东西了
用法:
$DBH = new PDO("mysql:host=$host;dbname=$dbname", $user, $pass);
$userRepository = new UserRepository($DBH);
$id = $_SESSION['MM_Username'];
try
{
$me = $userRepository->find($id);
}
catch( UserNotFoundException $e )
{
//user not found
}
print $me->getSurName();
用户类别:
class User
{
/**
* If the User persists in the DataBase
* $id holds it's db id
* @var int
*/
private $id;
/**
* @var String
*/
private $surName;
/**
* @var String
*/
private $firstName;
/**
* @param String $firstName
* @param String $surName
* @param int $dbId
*/
public function __construct($firstName, $surName, $dbId=null)
{
$this->id = $dbId;
$this->firstName = $surName;
$this->surName = $surName;
}
/**
* Does the user ecist in the DB?
* @return boolean
*/
public function hasId()
{
return $this->id !== null;
}
/**
* @return int
* @return null user doesn't persist in DB
*/
public function getId()
{
return $this->id;
}
/**
* Return the users full name
* The fullname consists of his FirstName and SurNAme
* @return String
*/
public function getName()
{
return $this->firstName . ' ' . $this->surName;
}
/**
* @return String
*/
public function getSurName()
{
return $this->surName;
}
/**
* @return String
*/
public function getFirstName()
{
return $this->firstName;
}
/**
* Setters: we return $this to allow chaning
*/
public function setId($id)
{
$this->id = $id;
return $this;
}
// ... other methods here. You can add extra swet stuff.
// for instance check or it is a valid firstName, or email or ...
//I removed your __destrouct, because wtf? it isn't doing anything at all
}
和您的 UserRepository:
/**
* The UserRepository queries the database and get's you your users
*/
class UserRepository
{
private $db;
public function __construct(PDO $db)
{
$this->db = $db;
}
public function find($id)
{
$statement = $this->db->prepare('SELECT id,first_name,sur_name FROM users WHERE id = :id');
$statement->execute(array(
'id' => $id
));
if ( null === ($user = $statement->fetch(PDO::FETCH_ASSOC)) )
{
throw new UserNotFoundException();
}
return new User(
$user['first_name'],
$user['sur_name'],
$user['id']
);
}
}
还有例外:
class UserNotFoundException extends Exception();