Probably because you are not stating session_start() anywhere ... that I can see anyhow.

Overall this login method (never mind the class) is taking on an awful lot of responsibility.

A class ought to do one single thing, and do it properly.

It is OK to have a class which does very little but tells lots of other smaller classes what to do, delegating and orchestrating if you will.

Here are some pseudocode-ish ideas, not to strip your class down to one single thing, but to achieve a bit of a halfway house, so you should still recognise what it does - but also appreciate how some of the complexity and rigidity can be stripped out.

class LoginValidator {
function __constructor(PDO $PDO ){
// set up your pdo as a member (property)
}
function checkCredentials($user ='', $password=''){
// just do one single select here ;
$query = "SELECT id, name, farmname, level FROM users WHERE email=:email AND password = :password";
on success -- tell $this->startSession($session_vars) to start the session
return true;
else (on fail)
return false;
}
private function startSession($session_vars){
// this imagines using another class which serves to be used in many places in your code
// either pass an array of values as an argument
// or access private members which checkCredentials() would set up for you
$s = new Session($session_vars);
}
}
// usage -- where $pdo is created elsewhere, it is already set up, you just pass it as an argument...
if( user and password are set ) {
$lv = new LoginValidator($pdo);
}
if( $lv->checkCredentials($_POST['user'], $_POST['password']) is false ){
NOW redirect the user to somewhere else
} else {
redirect the user back to the page
}

I am hinting at the existence of a simple Session class which does not yet exist, but is simple enough to code up (never mind find on the web somewhere)

Going back to the CheckCredentials class idea above (I have no idea what your class is called) even this name CheckCredentials is not accurate because as I have written it it is more accurately a CheckCredentialsAndStartSession class (I remember calling a similar class "Bouncer" -- as in a doorman )

But still, it is now orchestrating, delegating to other classes what to do.

It does one basic thing (sort of), it checks to see if an instance of user and pass exist in the dbIt delegates to a Session classIt is given an instance of a PDO which it needs to do its job (it should not care whether that is using a mysql or sqlite driver)It leaves the responsibility of redirecting the user to the userland coder, though this could be another class too ...

You will notice I have completely ignored your clean() method, I can only guess what that does and surmise that it is not adding anything to the party at all.

Notice it does NOT check whether user and pass are even set, it is pretty dumb in that respect. You could smarten it up, you could start adding an array of different error messages ... but they would differ depending on which application was using it -- the downside would be that it would start cluttering up your LoginValidator.

Now the trick is, can you alter LoginValidator slightly so that it is not forever tied to your users/farm application?

Then you can use it, like the Session class, in many more of your development tasks.

My example is not meant to be perfect by any means, just some ideas to get you thinking, if some of this makes sense to you and interests you then it could be time to do some OOP study.

Probably because you are not stating session_start() anywhere ... that I can see anyhow.

Overall this login method (never mind the class) is taking on an awful lot of responsibility.

A class ought to do one single thing, and do it properly.

It is OK to have a class which does very little but tells lots of other smaller classes what to do, delegating and orchestrating if you will.

Here are some pseudocode-ish ideas, not to strip your class down to one single thing, but to achieve a bit of a halfway house, so you should still recognise what it does - but also appreciate how some of the complexity and rigidity can be stripped out.

`php

class LoginValidator {

function __constructor(PDO $PDO ){ // set up your pdo as a member (property) }

private function startSession($session_vars){ // this imagines using another class which serves to be used in many places in your code // either pass an array of values as an argument // or access private members which checkCredentials() would set up for you

$s = new Session($session_vars); }

}

// usage -- where $pdo is created elsewhere, it is already set up, you just pass it as an argument...

if( user and password are set ) { $lv = new LoginValidator($pdo);}

if( $lv->checkCredentials($POST['user'], $POST['password']) is false ){ NOW redirect the user to somewhere else} else { redirect the user back to the page}

`

I am hinting at the existence of a simple Session class which does not yet exist, but is simple enough to code up (never mind find on the web somewhere)

Going back to the CheckCredentials class idea above (I have no idea what your class is called) even this name CheckCredentials is not accurate because as I have written it it is more accurately a CheckCredentialsAndStartSession class (I remember calling a similar class "Bouncer" -- as in a doorman )

But still, it is now orchestrating, delegating to other classes what to do.

It does one basic thing (sort of), it checks to see if an instance of user and pass exist in the dbIt delegates to a Session classIt is given an instance of a PDO which it needs to do its job (it should not care whether that is using a mysql or sqlite driver)It leaves the responsibility of redirecting the user to the userland coder, though this could be another class too ...

You will notice I have completely ignored your clean() method, I can only guess what that does and surmise that it is not adding anything to the party at all.

Notice it does NOT check whether user and pass are even set, it is pretty dumb in that respect. You could smarten it up, you could start adding an array of different error messages ... but they would differ depending on which application was using it -- the downside would be that it would start cluttering up your LoginValidator.

Now the trick is, can you alter LoginValidator slightly so that it is not forever tied to your users/farm application?

Then you can use it, like the Session class, in many more of your development tasks.

My example is not meant to be perfect by any means, just some ideas to get you thinking, if some of this makes sense to you and interests you then it could be time to do some OOP study.

Some good comments cups...

I was thinking, perhaps it might make sense to have the session class completely outside of this class, and in the calling code have something like:

I mean, yeah, now we're off ... thanks for replying, it underlines and supports a point I did not make very clear, that there are many ways of doing the same thing.

aaarrrggh said:

Etc... what do you reckon?

If it works for you then great. We could now go off and play with trade-offs between responsibilities, degrees of coupling and will no doubt feature factories vs Dependency Injection and all the OOP buzzwords.

This is all grist to the OOP mill, and I am happy to discuss for as far as my Pattern knowledge will carry me (which is NOT as advanced as many on here may think).

The thing is, I don't want to leave the OP behind when it is clear from the method example posted there are some fundamental OOP principles that are being broken and clearly the OP could do with some help to shed some light on what is going on with the code.

I don't want to come across as being pretentious here but even if it we can have the OP think (or anyone else watching -- why not say hello?), "Crikey, there is a bit more to this OOP lark than I first thought" and that he/she would come back and ask more questions about objects then that would be really, really great.

Because, you know what, some of the old OOP Gods that used to hang out on here might come back and share some of their wisdom.

I don't know about you, but when I start thinking about OOP, I find it helpful to imagine I am talking to my dogs.

It is better (when working with OO style) to break things down in to there own functions in side a class. Say if i was to make a document function and i wanted to validate that the title and document field could i use one class to validate both the title and document and then if both are valid have a variable returned as true then use that in a different class.

It is better (when working with OO style) to break things down in to there own functions in side a class.

A class should do one thing properly. It can have as many moving parts as you want (methods). You can have your class call its own methods.

Your documents class does not seem to do much at all, it checks that the title and the document themselves are not empty, but lets stay with it and look at a few of the options you could take into consideration.

Perhaps it would help to think about how you'd like to initiate and call the class methods from user land:

<?php
// presumably this is coming from a HTTP request ...
$document = "Incoming data forming the document"';
$title = "The document Title";
// $pdo this has been instantiated somewhere else, and is perhaps included

One difference is that in a) you leave the option open to store a document without a Title, or, in the future you could add a new field, Author, without having to go back and change the insides of your DocumentStorer by just doing $storer->checkNotNull($author);

Some might thing this is academic, e.g. why would you even want to instantiate a class when a simple empty() check on the incoming variables prior to starting, but we will leave that aside for the sake of illustrating my points.

Now you could also do c) which is what I think you are trying to do, the big single bang.

// feed the class everything it needs in one go
$storer = new DocumentStorer($pdo, $document, $title);
// I don't much care how you do it, either be successful or get an error message back
if( $storer->save() === false ){
echo $storer->getMessages();
}

How you would build your class would then depend on whether you want to do a) b) or c) (or d, e, f for that matter).

The code to satisfy c) would look something like this, I put some echo statements in there so you can see what is happening, you can add getMessages() or a getErrors() method yourself.

Thanks! You guys (And gals?) on SPF are the best! Now i like the idea of a, checking to see if it is null with checkNotNull and think it would be way easier to add stuff in the future if i needed to.

Well, a guy here actually. You are free to rename the methods to something that means more to you if you want, I just found it easier to rewrite what you had as a way of illustrating what could be done than to line by line tell you where I think you were going wrong.

There are just so many ways of approaching your particular problems, but yes, your reply "... think it would be way easier to add stuff in the future if i needed to." makes me very happy

Cmarenburg said:

Can any one recomend a OO php book. I have a few from SP but would deffently like to read more.

There have been lots of threads about OOP books if you search this forum, but it'd be interesting to see if there are any new ones.

I have not read them all, so have nothing to add to those old forum posts you should find.