smpl/mydi

Предложения по рефакторингу

Closed this issue · 6 comments

smpl commented

Тут вместо использования array_push() для добавления одного элемента в массив, лучше использовать $this->calls[] = $name;, потому что в этом случае не происходит затрат на вызов функции.
https://github.com/smpl/mydi/blob/master/src/Locator.php#L47

Тут https://github.com/smpl/mydi/blob/master/src/Locator.php#L68-L72 было бы проще написать такой код, не создавая лишнюю переменную $result
if (!empty($this->calls)) {
return $this->calls[count($this->calls) - 1];
}
return $name;

Тут https://github.com/smpl/mydi/blob/master/src/Locator.php#L106-L113 лучше написать такой код
foreach ($this->getContainers() as $loader) {
if ($loader->has($name)) {
return $loader;
}
}

Тут https://github.com/smpl/mydi/blob/master/src/loader/ObjectTrait.php#L49 лучше думаю не создавать лишнюю переменную $class, а написать сразу в static

Тут https://github.com/smpl/mydi/blob/master/src/container/IoC.php#L41-L48 лучше написать такой код
if (is_string($containerName)) {
$path = $this->containerNameToPath($containerName);
if (substr($path, 0, strlen($this->basePath)) === $this->basePath) {
return is_readable($this->containerNameToPath($containerName));
}
}
return false;

Тут тоже самое https://github.com/smpl/mydi/blob/master/src/container/KeyValueJson.php#L19-L20 перенести в return

smpl commented

Первое замечание логично и в различных бенчмарках оно быстрее

smpl commented

По поводу второго предложения:
В целом новая переменная будет но данные копироваться не будут подробней можно почитать про оптимизацию copy on write например тут http://www.phpinternalsbook.com/zvals/memory_management.html

Поэтому остается чисто замечания по внешнему виду метода, делать 2 выхода из метода не очень хорошо , где то в умных книжках читал чтобы был только один по возможности.

smpl commented

Третье предложение
Проверил ситуацию чтобы если функция ничего не возвращает, то php считает что вернуло null (нету void) https://3v4l.org/2YmJF
Поэтому да так более красиво и понятно, но phpstorm говорит:
Warning:(111, 5) Missing return statement
/home/smpl/projects/mydi/src/Locator.php
Что типо надо вернуть что-то так что пусть будет как было.

smpl commented

4 предложение
Вообщем тут опять из за copy on write не сильно потеряем, но для красоты лучше сделать как предлагают.

smpl commented

5 предложение

Делать не буду множественный выход из метода, а пользы практически нет (смотри 2 предложение)

smpl commented

6 предложение сделано.