How to split responsibility between classes

[AntonZelenin]

I'm creating a chat app. Right now I'm working on registration/authorization. I have classes: Guard, Database, Cookie and Registration.

Guard checks if user has valid cookie or login and password. But I can't decide what each class should be responsible for. For example should Guard set and unset cookie for user, or Cookie class should do it. Also Guard could get user tokens from db and add a new one.

And how to make user input safe for creating new user? Guard is responsible for user input filtering, but it has lots of methods that are not needed for Registration class.

See attachments)

 

Thanks in advance:)

Edited May 30, 2017 by AntonZelenin

[Jacques1]

Don't give us a bunch of screen shots of code where the interesting parts aren't even visible. Insert the code as text into code tags.

 

Your "Guard" class is so vague and so bloated that it should be removed altogether. A class should do one thing and one thing only.

 

At the same time, your "Cookie" class is far too specialized -- or misnamed, really. This is not about a cookie. It's about your specific user cookie. And then again, what is a random number generator doing there? The class should use a generator, not implement one. Note that PHP itself has a randomness API, so don't reinvent the wheel.

 

Your database class also looks like a weird reinvention. Nowadays, we have the PDO extension which already has all those features and supports prepared statements to safely pass input to queries.

[AntonZelenin]

Don't give us a bunch of screen shots of code where the interesting parts aren't even visible. Insert the code as text into code tags.

 

Your "Guard" class is so vague and so bloated that it should be removed altogether. A class should do one thing and one thing only.

 

At the same time, your "Cookie" class is far too specialized -- or misnamed, really. This is not about a cookie. It's about your specific user cookie. And then again, what is a random number generator doing there? The class should use a generator, not implement one. Note that PHP itself has a randomness API, so don't reinvent the wheel.

 

Your database class also looks like a weird reinvention. Nowadays, we have the PDO extension which already has all those features and supports prepared statements to safely pass input to queries.

As random generator I use exactly random_bytes();

public function generate_token($length = 20) {
    return bin2hex(random_bytes($length));
}

And for database connection I use MySQLi. That methods I created just to shorten some operations, for example:

public function query($query) {
    $query_result = $this->connection->query($query);
    if(!$query_result) {
        throw new Exception("Can't perform a query ".$query."\r\n");
    }else{
        return $query_result;
    }
}

and

public function fetch_all_array($query_result) {
    for($set = array(); $row = $query_result->fetch_row(); $set[] = $row);
    return $set;
}

Is it okay?

 

"One thing only" is a bit unclear for me, so I'm trying to comprehend how a good class should look like)

I'll try to remake my classes tomorrow and send you a result. Maybe I'll do better this time)

[Jacques1]

As random generator I use exactly random_bytes();

 

Great. But this belongs into a token class, not a cookie class.

 

It's not the job of a cookie to generate random bytes. A cookie may use random bytes, but how exactly they're generated is somebody else's responsibility.

 

This may seem like nitpicking to you, but it's a very real problem: When a particular feature is implemented in all kinds of (unexpected) places rather than one central class, you quickly end up with code duplication, inconsistencies and maintainability problems. For example, the randomness API didn't even exist until PHP 7 was released, so if you had written the code just a bit earlier, you would probably have used mcrypt_create_iv() or openssl_random_pseudo_bytes(). Now imagine doing an update to random_bytes() when this stuff is everywhere in your code: the cookie class, the registration component, the password reset component and who knows where else. That's going to be a long, painful procedure.

 

On the other hand, if you have a single token class, you can change the implementation details of random number generation at any time, and the users of the class won't even notice.

 

 

 

And for database connection I use MySQLi.

 

I was afraid you would say that.

 

MySQLi is a nasty low-level interface which indeed requires tons of boilerplate code and wrappers to make it somewhat usable. If you have any chance to switch to PDO, do it. You won't need those wrappers anymore, because PDO has a lot of convenience methods (fetch all rows, fetch a column, fetch a single value etc.) built in.

 

Actually, even mysqli has fetch_all(), so the code is superfluous in any case.

 

And like I said, you should learn how to use prepared statements rather than manually escaping input.

 

 

 

"One thing only" is a bit unclear for me, so I'm trying to comprehend how a good class should look like)

 

A class should have a single well-defined responsibility. You should be able to describe it in one short sentence.

 

This is true for your Database class (ignoring the other problems). It isn't true at all for your Guard class (which already has a suspiciously vague name). The “Guard” does all kinds of things: input escaping, cookie management, user management. There's no clear purpose. It's a loose collection of different features.

Edited May 30, 2017 by Jacques1