Minor cleanup to search in-memory index #8148
Conversation
| final Map<String, PackageDocument> _packages = <String, PackageDocument>{}; | ||
| final _packageNameIndex = PackageNameIndex(); | ||
| final Map<String, PackageDocument> _packages = {}; | ||
| final PackageNameIndex _packageNameIndex = PackageNameIndex(); |
There was a problem hiding this comment.
Not sure: why do we need to specify the type here?
| /// Adjusted score takes the overall score and transforms | ||
| /// it linearly into the [0.4-1.0] range. | ||
| final _adjustedOverallScores = <String, double>{}; | ||
| final Map<String, double> _adjustedOverallScores = {}; |
There was a problem hiding this comment.
Same here: type inference seems to be enough, why the extra specification?
There was a problem hiding this comment.
The inference is enough, but this change allows it to be consistent with the other nearby field entries.
Also, since these are fields and can be used outside of the direct vicinity, I generally think it makes sense to specify (and as a result, explicitly think about) the type.
Happy to revert these changes though :)
There was a problem hiding this comment.
Oh, now I understand the intent here. The other nearby field entries are older code that preceded the inference capabilities in the language, so if we wanted to make them consistent, I'd rather move them towards using the more modern features. wdyt?
| _updateOverallScores(); | ||
| _lastUpdated = clock.now().toUtc(); | ||
| _overallOrderedHits = _rankWithComparator(_compareOverall, | ||
| _overallOrderedHits = _rankWithComparator(_packages, _compareOverall, |
There was a problem hiding this comment.
Not sure: is there any benefit of extracting these methods outside of the class? It is not that we will reuse them in other places.
There was a problem hiding this comment.
This was more of a transitionary change to a world where a lot of the mutable state doesn't have to escape construction. But that proved too large, so I thought this initial change would still be an improvement for now while getting a bit closer.
I do think making it static helps make it clear (and enforces) that it's not impacting any state of the class.
|
Let's put this PR aside, as I started to look into a refactoring that would (a) include many of the changes here, and (b) be in conflict with most of it. |
2 participants
Primarily moves some declarations to be
static, to reduce the large amount of instance members that end up in scope for those methods. This will make the route to the instance fields no longer beinglatea bit easier.Also avoids some unnecessary non-null checks and assertions.