Enhancement: Improve legibility of ScopeManager #35
Conversation
src/DDTrace/ScopeManager.php
Outdated
| } | ||
|
|
||
| return $this->scopes[count($this->scopes) - 1]; | ||
| return end($this->scopes); |
There was a problem hiding this comment.
Apparently, we are interested in returning the last element, so maybe end() is better?
There was a problem hiding this comment.
end() modifies the array's internal pointer, which may or may not surprise us somewhere else. I don't think the original line is unclear.
| public function deactivate(Scope $scope) | ||
| { | ||
| $scopeLength = count($this->scopes); | ||
| $i = array_search($scope, $this->scopes, true); |
There was a problem hiding this comment.
We could use array_search() to find the index of $scope in $this->scopes, maybe better than looping?
chuck
left a comment
chuck
left a comment
There was a problem hiding this comment.
The array_search() works, but I don't think the end() one is a good idea.
src/DDTrace/ScopeManager.php
Outdated
| } | ||
|
|
||
| return $this->scopes[count($this->scopes) - 1]; | ||
| return end($this->scopes); |
There was a problem hiding this comment.
end() modifies the array's internal pointer, which may or may not surprise us somewhere else. I don't think the original line is unclear.
localheinz
force-pushed
the
feature/scope-manager
branch
from
657b38c to
a968547
Compare
|
@localheinz Can you update based on feedback, or should I work off of your branch? |
|
Already amended a couple days ago - does this work? Please let me know if you prefer me to apply more changes. 🤓 |
|
Sorry, missed the update. Thanks! |
|
Thank you, @chuck! |
2 participants
This PR
ScopeManagera bit