Selection: moved caching related functionality to separate classes #185
Conversation
|
Thanks for pull request. If I understand it, this is a pure refactoring that separates the code into new classes but does not change the functionality? |
|
@dg Yes, exactly :) |
| } | ||
|
|
||
|
|
||
| public function setGeneralCacheKey(?string $key) |
There was a problem hiding this comment.
Because it is targeted to 7.1, you can use return type void.
|
|
||
|
|
||
| /** | ||
| * @param mixed $accessedColumns |
There was a problem hiding this comment.
Please don't use property name in @param. I know that PhpStorm appends it automatically, but better is to be consistent with rest of code.
Btw is it really mixed or array?
|
|
||
| /** | ||
| * Loads cache of previous accessed columns and returns it. | ||
| * @return array|bool |
There was a problem hiding this comment.
This is just move from original class. I wasn't sure so I reather left it as is.
There was a problem hiding this comment.
There are more variables that use null and bool to distinguish some states. I would like to fix this in second run.
There was a problem hiding this comment.
It seems that array_keys() can return only array, or not?
There was a problem hiding this comment.
Yes, and it passes tests, fixed.
|
|
||
|
|
||
| /** | ||
| * @param mixed $observeCache |
There was a problem hiding this comment.
Here iare stored Selection instances.
|
Rest fixed |
|
|
||
|
|
||
| /** | ||
| * @param mixed |
There was a problem hiding this comment.
$previousAccessedColumns has typehint array and here is mixed
There was a problem hiding this comment.
My bad. I copied properties as they were. Fixed on all properties.
| /** | ||
| * @param mixed | ||
| */ | ||
| public function setAccessedColumns($accessedColumns): void |
There was a problem hiding this comment.
$accessedColumns has typehint array and here is mixed
|
|
||
|
|
||
| /** | ||
| * @return mixed |
There was a problem hiding this comment.
$accessedColumns has typehint array and here is mixed
|
|
||
|
|
||
| /** | ||
| * @param mixed |
There was a problem hiding this comment.
$observeCache has typehint bool and here is mixed
There was a problem hiding this comment.
Fixed, allowed only Selection.
|
@dg Anything else I can do so you merge it? |
| /** @var string */ | ||
| protected $specificCacheKey; | ||
|
|
||
| /** @var mixed of touched columns */ |
There was a problem hiding this comment.
What means mixed of? Is it array or not?
There was a problem hiding this comment.
There was lot of mess arround mixed properties, so I get rid of them and simplify code in separate commit. Let me know what you think.
|
|
||
|
|
||
| /** | ||
| * @return mixed |
There was a problem hiding this comment.
It seems according to the code that it is array|null, or not? For getReferenced too.
There was a problem hiding this comment.
Yes, you are right
|
|
||
| public function setAccessedColumn(string $key, bool $value): void | ||
| { | ||
| if (!$this->cache) { |
There was a problem hiding this comment.
Just such a stupid thing, maybe positive condition can be used here and in setAccessedColumns
if ($this->cache) {
$this->accessedColumns[$key] = $value;
}
```There was a problem hiding this comment.
Fixed on both places.
|
Now it look much better, thanks! |
| /** | ||
| * @return array|null | ||
| */ | ||
| public function &getReferencing(string $generalCacheKey) |
There was a problem hiding this comment.
Sry, now I realized that it can be written native as &getReferencing(string $generalCacheKey): ?array
There was a problem hiding this comment.
Yes, I should realize that too. Fixed
|
Do you want to have two commits, one with mixed and the other without mixed properties? |
|
Thanks, great! |
@dg Cache implementation is a long term problem in
NDBT. While this is definitely not perfect, it's a starting point from which I would like to continue makingNDBTcode more clear, which wil help in future development. I spent two days with this, so I hope you appreciate it. Also, if you agree, I can maintain this repo as we spoken on NettCamp. There is lot for review and merge.