[PM-14476] Avoid multiple lookups in dictionaries #4973
Conversation
|
Oh, and there are a few places where I avoid allocating a new empty array or list |
|
Thank you for your contribution! We've added this to our internal Community PR board for review. |
bitwarden-bot
changed the title
| if (providers == null || !providers.TryGetValue(provider, out var twoFactorProvider)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| return providers[provider].Enabled && Use2fa; | ||
| return twoFactorProvider.Enabled && Use2fa; | ||
| } |
There was a problem hiding this comment.
🎨 This could be inverted for clarity. Something like this might be better.
public bool TwoFactorProviderIsEnabled(TwoFactorProviderType provider)
{
var providers = GetTwoFactorProviders();
if (providers?.TryGetValue(provider, out var value) ?? false)
{
return value.Enabled && Use2fa;
}
return false;
}
There was a problem hiding this comment.
I went for minimum changes in lines of code, to avoid reviewing logical changes as well.
And I believe that guard clauses should be in top, to be consistent with the rest of the code.
It is also my personal preference
| if (providers is null || !providers.TryGetValue(type, out var provider)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| providers[type].Enabled = true; | ||
| provider.Enabled = true; |
There was a problem hiding this comment.
🎨 Inverting this might help with clarity
Something like this maybe:
var providers = organization.GetTwoFactorProviders();
if (providers?.TryGetValue(type, out var twoFactorProvider) ?? false)
{
twoFactorProvider.Enabled = true;
organization.SetTwoFactorProviders(providers);
await UpdateAsync(organization);
}
| { | ||
| _attachmentContainers[containerName] = _blobServiceClient.GetBlobContainerClient(containerName); | ||
| attachmentContainer = _blobServiceClient.GetBlobContainerClient(containerName); | ||
| _attachmentContainers[containerName] = attachmentContainer; |
There was a problem hiding this comment.
🎨 The container shouldn't be added to the dictionary until it is successfully created.
There was a problem hiding this comment.
In the existing code, it is already added before the creation.
If you are worried about concurrency issues, the _attachmentContainers Dictionary should also be changed to be a ConcurrentDictionary to avoid concurrency issues
If concurrency should be supported, I suggest doing it in a separate pull request
🎟️ Tracking
I looked at the code and saw there was a lot of unneeded dictionary lookups.
I want to remove these to avoid spending CPU time on generating hash values for lookups.
📔 Objective
Improve performance + scalability plus reduce carbon impact.
I know that I have touched a lot of files, but I was easier to just do it everywhere, than to find the hot paths and only fix it there.
A benefit is that the code is more correct everywhere. ⛏
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes