At multiple places in the codebase do you run the session_start() this function should only be run once, and only if the session is not running, calling isUserOnline() and destroySession() would result in a PHP Notice: A session had already been started - ignoring session_start() in php shell code on line 1, other combinations of session related method would also end up in this. Consider reading this question about checking if the session is running question about checking if the session is running
At multiple places in the codebase do you run the session_start() this function should only be run once, and only if the session is not running, calling isUserOnline() and destroySession() would result in a PHP Notice: A session had already been started - ignoring session_start() in php shell code on line 1, other combinations of session related method would also end up in this. Consider reading this question about checking if the session is running
At multiple places in the codebase do you run the session_start() this function should only be run once, and only if the session is not running, calling isUserOnline() and destroySession() would result in a PHP Notice: A session had already been started - ignoring session_start() in php shell code on line 1, other combinations of session related method would also end up in this. Consider reading this question about checking if the session is running
Each concrete (Request, Response, Mapper) could be replaced with a Interface hint making the code more flexible for changingchanges in the dependency and not relying on a concrete classavoiding tight coupling.
When I read efficient, I understand that as fast, if that is the case, do not worry about it, that could be considered as premature optimization. There might be some places where you couldone might reconsider if the logic is really justified, readblereadable and easy to refactor, again, it really has nothing to do with speed, but consider the following code from above:.
I most of your code, most ofAccess to the globals has been nicely$_SESSION should be abstracted away into a class. You should also do this to you $_SESSION as you have done with the $_GET/$_SET
- Have I written SOLID code? ================================== If by SOLID code you mean that it must adhereSingle responsibility principle - There seems to eachbe abit of trouble adhering to this principle especially in the Mapper, noit acts both as a proxy to the user, wrapper for the session. It should take a map a user not depend on one via. the constructor.
Single responsibilityDependency inversion principle - There seems to be abit of trouble adhering to this princple especially in the Mapper, it acts both as a proxy to the user, wrapper for the session. It should take a map a user not depend on one via.Remove your hard dependencies from the constructorconstructors and add interfaces instead.
As mentioned earlier you are not using the Dependency inversion principle.
__construct(FooInterface $foo = null)
{
$this->foo = $foo ?: new MyDefaultFoo;
}
Each concrete (Request, Response, Mapper) could be replaced with a Interface hint making the code more flexible for changing the dependency and not relying on a concrete class.
When I read efficient, I understand that as fast, if that is the case, do not worry about it, that could be considered as premature optimization. There might be some places where you could reconsider if the logic is really justified, readble and easy to refactor, again, it really has nothing to do with speed, but consider the following code from above:
I most of your code, most of the globals has been nicely abstracted away. You should also do this to you $_SESSION as you have done with the $_GET/$_SET
- Have I written SOLID code? ================================== If by SOLID code you mean that it must adhere to each principle, no.
Single responsibility principle - There seems to be abit of trouble adhering to this princple especially in the Mapper, it acts both as a proxy to the user, wrapper for the session. It should take a map a user not depend on one via. the constructor.
As mentioned earlier you are not using the Dependency inversion principle.
Each concrete (Request, Response, Mapper) could be replaced with a Interface making the code more flexible for changes in the dependency and not avoiding tight coupling.
When I read efficient, I understand that as fast, if that is the case, do not worry about it, that could be considered as premature optimization. There might be some places where one might reconsider if the logic is really justified, readable and easy to refactor, again, it really has nothing to do with speed.
Access to the $_SESSION should be abstracted away into a class.
- Have I written SOLID code? ================================== Single responsibility principle - There seems to be abit of trouble adhering to this principle especially in the Mapper, it acts both as a proxy to the user, wrapper for the session. It should take a map a user not depend on one via. the constructor.
Dependency inversion principle - Remove your hard dependencies from the constructors and add interfaces instead.
__construct(FooInterface $foo = null)
{
$this->foo = $foo ?: new MyDefaultFoo;
}
- How may I make it more flexible? ================================== Depend on abstractions not on concretions (SOLI D )
__construct(Request $request, Response $response, Mapper $mapper)
Each concrete (Request, Response, Mapper) could be replaced with a Interface hint making the code more flexible for changing the dependency and not relying on a concrete class.
- Is my code efficient? How may I make it more efficient? ==================================
When I read efficient, I understand that as fast, if that is the case, do not worry about it, that could be considered as premature optimization. There might be some places where you could reconsider if the logic is really justified, readble and easy to refactor, again, it really has nothing to do with speed, but consider the following code from above:
session_start();
unset($_SESSION['username']);
unset($_SESSION['logoutTime']);
session_destroy();
session_start();
session_destroy();
Reconsider the logic in this code, take a extra look at session_destroy and the example on removing a session.
Also single $_SESSION = []; will remove any data.
- Do you have any recommendations for my code? ==================================
I most of your code, most of the globals has been nicely abstracted away. You should also do this to you $_SESSION as you have done with the $_GET/$_SET
A couple of places in your code return a boolean, consider using no return value when none is needed and istead throw a Exception if something went wrong.
- Have I written SOLID code? ================================== If by SOLID code you mean that it must adhere to each principle, no.
Single responsibility principle - There seems to be abit of trouble adhering to this princple especially in the Mapper, it acts both as a proxy to the user, wrapper for the session. It should take a map a user not depend on one via. the constructor.
As mentioned earlier you are not using the Dependency inversion principle.
Take care, as it is very hard to forfill these principles at all times, I would recommend to strive for them but accept it's not always what you end up with. Also there are other 3 and 5 letter acronyms also worth remembering and striving for. =]
- Do you see any flaws in my code? If so please point out. ==================================
At multiple places in the codebase do you run the session_start() this function should only be run once, and only if the session is not running, calling isUserOnline() and destroySession() would result in a PHP Notice: A session had already been started - ignoring session_start() in php shell code on line 1, other combinations of session related method would also end up in this. Consider reading this question about checking if the session is running
Consider the difference between private and protected. Also keep in mind why you close of access to modification, if you make both a setValue($x) { $this->x = $x } and getValue($x) { return $this->x; } having a private $x; has no use and might as well be a public $x.
Is the system secure? If not, how may I make it more secure? ================================== I am not a security expert and will leave any real security advice to a expert any day of the week. If you need security review I would recommend a question with a really specific context. The only things I can take away from the code and give some comments one is
In any security/permission related context always use
===instead of==regardingpassword_verify($rawPassword, $databasePassword) == true, but in this case it does not really matter, just do not make it a habit.Watch out for
while($query->fetch()only one user should be fetched and the username should be a unique value.