Aded Data Helper and Session Helper #12
Conversation
| * @var PCRE | ||
| * | ||
| */ | ||
| private static $binary_regexp = '/^(?:[01]{8}){0,12}$/'; // UNTESTED |
There was a problem hiding this comment.
we should avoid static whenever possible in Ohanzee.
There was a problem hiding this comment.
What do you prefer in that instance? const??
Context: I strayed away from 'const' originally because the data would be public, I guess it seems trivial for it not to be in this case.
There was a problem hiding this comment.
I also, notice that you are calling local methods using static::method(), do you prefer that to self::method(). Not sure if there's a need for static::method() in a no dependency library since there should be no late static bindings. Unless you foresee users extending these classes in such a way.
There was a problem hiding this comment.
replaced static props with constants where possible.
|
It would be really helpful if these new classes came with phpspec tests. |
|
Sounds good to me. I'll get some setup. |
|
Also, just reviewed the psr-2 spec and realized I have some solid changes to make. Should be up to spec in a day or two. |
| */ | ||
| public static function isActive() | ||
| { | ||
| return self::status() === PHP_SESSION_ACTIVE ? true : false; |
There was a problem hiding this comment.
here, the expression already returns boolean, you don't need the ternary at all.
…id during active session
|
I think I've addressed all of your concerns from the last code review. I still need to get on some phpspec tests, though. |
|
@shadowhand it would be possible to merge this and make a ticket for tests. |
|
It should be composed of two commits, not 26. And it needs tests. |
These are two objects I find myself using a lot. I feel like the Class Name "Data" might be a bit too generic and would advise an alternate name, nothing came to mind right away.