Session Class

[abiusx]

1. When a function throws exceptions, it should indicate them in its PHPDOC at the top using @throws headers. This way uses will know what possible errors occur in the method, and handle them appropriately without looking into the source code.
2. You have too much code, specially in getters and setters, that are never used. setInactivityTimeout and stuff like that, they just make the code look complicated. We also don't do type checking in PHP unless necessary. Let it error instead of checking it.

Just assign things to properties and all, no getters and setters unless some real magic is needed.

1. Throw base \Exception very rarely, and only when the application really needs to die. No one can catch and handle them properly.
2. Never use underscores in method names.
3. Add comments for properties, in PHPDOC. They must at least define types.
4. I don't get why you are using try...catch blocks in the code. You are not using some other library that throws error! Its futile.
5. No need for destructor. The thing you're trying to achieve happens automatically.
6. Remove underscore from property names, and make them public if they are supposed to be changeable by the user!
7. Use private only when its private. Most of what you're doing needs protected. How to know? Imagine one class inherits from your class to add some functionality, does it need access to something? if yes, go protected.

Thats all for now :D

[rash805115]

Hi...if you are here...I have doubts regarding point 4:

I am using the DB library for SQL commands. So if some exception arises there, then I am just catching that exception and throwing back to user so that they can handle them the way they want. Is it wrong. What changes do I need to make ?

On Sun, Jun 30, 2013 at 12:52 PM, AbiusX notifications@github.com wrote:

1.

When a function throws exceptions, it should indicate them in its PHPDOC at the top using @throws https://github.com/throws headers. This way uses will know what possible errors occur in the method, and handle them appropriately without looking into the source code. 2.

You have too much code, specially in getters and setters, that are never used. setInactivityTimeout and stuff like that, they just make the code look complicated. We also don't do type checking in PHP unless necessary. Let it error instead of checking it.

Just assign things to properties and all, no getters and setters unless some real magic is needed.

1.

Throw base \Exception very rarely, and only when the application really needs to die. No one can catch and handle them properly. 2.

Never use underscores in method names. 3.

Add comments for properties, in PHPDOC. They must at least define types. 4.

I don't get why you are using try...catch blocks in the code. You are not using some other library that throws error! Its futile. 5.

No need for destructor. The thing you're trying to achieve happens automatically. 6.

Remove underscore from property names, and make them public if they are supposed to be changeable by the user! 7.

Use private only when its private. Most of what you're doing needs protected. How to know? Imagine one class inherits from your class to add some functionality, does it need access to something? if yes, go protected.

Thats all for now :D

— Reply to this email directly or view it on GitHubhttps://github.com/OWASP/phpsec/issues/8 .

Regards, Rahul Chaudhary Ph - 412-519-9634

[rash805115]

Remaining:

1) Throw base \Exception very rarely, and only when the application really needs to die. No one can catch and handle them properly.

2) I don't get why you are using try...catch blocks in the code. You are not using some other library that throws error! Its futile.

[abiusx]

Well the DB class is already throwing them, so one can catch those. It doesnt matter if its more nested, as long as its run between the try...catch block, it will be catched. You don't need to rethrow it.

The only scenario that you rethrow something, is when you have a general catcher which catches everything, and you want some particular error to be rethrown, and it rarely happens.

[rash805115]

Hmm....makes sense...so I can rub away all the tru catch blocks now...

On Mon, Jul 1, 2013 at 3:27 PM, AbiusX notifications@github.com wrote:

Well the DB class is already throwing them, so one can catch those. It doesnt matter if its more nested, as long as its run between the try...catch block, it will be catched. You don't need to rethrow it.

The only scenario that you rethrow something, is when you have a general catcher which catches everything, and you want some particular error to be rethrown, and it rarely happens.

— Reply to this email directly or view it on GitHubhttps://github.com/OWASP/phpsec/issues/8#issuecomment-20304405 .

Regards, Rahul Chaudhary Ph - 412-519-9634

[rash805115]

Please check the session class if it has any more problems.

On Mon, Jul 1, 2013 at 3:29 PM, rahul chaudhary < rahul300chaudhary400@gmail.com> wrote:

Hmm....makes sense...so I can rub away all the tru catch blocks now...

On Mon, Jul 1, 2013 at 3:27 PM, AbiusX notifications@github.com wrote:

Well the DB class is already throwing them, so one can catch those. It doesnt matter if its more nested, as long as its run between the try...catch block, it will be catched. You don't need to rethrow it.

The only scenario that you rethrow something, is when you have a general catcher which catches everything, and you want some particular error to be rethrown, and it rarely happens.

— Reply to this email directly or view it on GitHubhttps://github.com/OWASP/phpsec/issues/8#issuecomment-20304405 .

Regards, Rahul Chaudhary Ph - 412-519-9634

Regards, Rahul Chaudhary Ph - 412-519-9634