patternphpMinor
Authentication service
Viewed 0 times
serviceauthenticationstackoverflow
Problem
This is the
I would like it mainly reviewed on a couple of aspects:
AuthenticationService.php
```
namespace Model\Application;
use Http\HttpSession;
use Model\Infrastructure\Mapper\UserMapper;
use Model\Application\AuthenticationStorage;
/**
* An authentication service.
*
* @author John Doe
*/
class AuthenticationService
{
/**
* @var HttpSession $httpSession An HttpSession instance.
*/
private $httpSession;
/**
* @var UserMapper $userMapper A UserMapper instance.
*/
private $userMapper;
/**
* @var AuthenticationStorage $authenticationStorage An AuthenticationStorage instance.
*/
private $authenticationStorage;
/**
* Constructs the object.
*
* @param HttpSession $httpSession An HttpSession instance.
* @param UserMapper $userMapper A UserMapper instance.
* @param AuthenticationStorage $authenticationStorage An AuthenticationStorage instance.
*/
public function __construct(
HttpSession $httpSession,
UserMapper $userMapper,
AuthenticationStorage $authenticationStorage
) {
$this->httpSession = $httpSession;
$this->userMapper = $userMapper;
$this->authenticationStorage = $authenticationStorage;
}
/**
* Finds the authenticated User entity.
*
* @return User|void|null The User entity instance if found.
*/
public function findAuthenticatedUser()
{
if ($identifier = $this->httpSession->findParameter('userIdentifier')) {
return $this->userMapper->findByIdentifier($identifier);
}
if ($user = $this->authenticationStor
AuthenticationService I made based on Barry Jaspan's design, which is way better than the code of which I asked in a question a couple of months ago.I would like it mainly reviewed on a couple of aspects:
- Readability (clean code)
- Accurate comments
- Accurate naming (especially for the last class)
AuthenticationService.php
```
namespace Model\Application;
use Http\HttpSession;
use Model\Infrastructure\Mapper\UserMapper;
use Model\Application\AuthenticationStorage;
/**
* An authentication service.
*
* @author John Doe
*/
class AuthenticationService
{
/**
* @var HttpSession $httpSession An HttpSession instance.
*/
private $httpSession;
/**
* @var UserMapper $userMapper A UserMapper instance.
*/
private $userMapper;
/**
* @var AuthenticationStorage $authenticationStorage An AuthenticationStorage instance.
*/
private $authenticationStorage;
/**
* Constructs the object.
*
* @param HttpSession $httpSession An HttpSession instance.
* @param UserMapper $userMapper A UserMapper instance.
* @param AuthenticationStorage $authenticationStorage An AuthenticationStorage instance.
*/
public function __construct(
HttpSession $httpSession,
UserMapper $userMapper,
AuthenticationStorage $authenticationStorage
) {
$this->httpSession = $httpSession;
$this->userMapper = $userMapper;
$this->authenticationStorage = $authenticationStorage;
}
/**
* Finds the authenticated User entity.
*
* @return User|void|null The User entity instance if found.
*/
public function findAuthenticatedUser()
{
if ($identifier = $this->httpSession->findParameter('userIdentifier')) {
return $this->userMapper->findByIdentifier($identifier);
}
if ($user = $this->authenticationStor
Solution
In
Instead of rebuilding
move these outside of the method and do it only once.
This is especially important because you have these duplicated in
The logic of joining and splitting cookie values by
It would be better to encapsulate that logic in a dedicated class,
which hides such details as the
A minor thing, but in a void method, why not simply
storeAuthenticationData, these variables are constants:// Character pool for CryptoRandCharGen::generateString().
$lowerAlpha = 'abcdefghijklmnopqrstuvwxyz';
$upperAlpha = strtoupper($lowerAlpha);
$digits = '0123456789';
$symbols = './<>?;"\'`!@#$%^&*()[]{}_+=|\-';
$characters = $lowerAlpha . $upperAlpha . $digits . $symbols;Instead of rebuilding
$characters every time this method is called,move these outside of the method and do it only once.
This is especially important because you have these duplicated in
updateAuthenticationData too.The logic of joining and splitting cookie values by
: appear at multiple places.It would be better to encapsulate that logic in a dedicated class,
which hides such details as the
: separator from the rest of the implementation.A minor thing, but in a void method, why not simply
return; instead of return null; ?Code Snippets
// Character pool for CryptoRandCharGen::generateString().
$lowerAlpha = 'abcdefghijklmnopqrstuvwxyz';
$upperAlpha = strtoupper($lowerAlpha);
$digits = '0123456789';
$symbols = './<>?;"\'`!@#$%^&*()[]{}_+=|\-';
$characters = $lowerAlpha . $upperAlpha . $digits . $symbols;Context
StackExchange Code Review Q#64087, answer score: 5
Revisions (0)
No revisions yet.