Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor experiments local var naming #1271

Merged
merged 2 commits into from
Aug 20, 2020

Conversation

pda
Copy link
Member

@pda pda commented Aug 19, 2020

Two parts, both rather superficial…


Simplify the implementation of experiments.IsEnabled(). Go's map type returns the zero-value of its value type for missing keys; so map[string]bool returns false for missing keys.

That means we don't need any extra logic to check for the existence of the key.


The rest of the PR is mostly var renaming to make the implementation of Enabled() clearer.

Idiomatic Go uses ok to indicate whether a key existed in a map, e.g. value, ok := experiments["foo"].
Whereas we were using it to refer to the boolean value of the map[string]bool in the context of a range,
i.e. for key, ok := range experiments.

And var enabled []string as the accumulator list of enabled experiments is problematic because enabled isn't exclusively plural; the same enabled name could also refer to a boolean value in the experiments map[string]bool. So this patch renames the list to keys, and also uses key instead of experiment everywhere to refer to experiment keys.

With the name enabled no longer being used for a list of enabled keys, this patch uses enabled instead of ok, resulting in a clearer if enabled { ... }.

pda added 2 commits August 19, 2020 17:08
This is mostly to make the implementation of Enabled() clearer.

Idiomatic Go uses `ok` to indicate whether a key _existed_ in a map,
e.g. `value, ok := experiments["foo"]`.
Whereas we were using it to refer to the boolean _value_ of the map in
the context of a range, i.e. `for key, ok := range experiments`.

And `var enabled []string` as the accumulator list of keys which are
enabled is problematic because it isn't plural, and the same `enabled`
name could also refer to a boolean value in the `map[string]bool`. So
this patch renames the list to `keys`, and also uses `key` instead of
`experiment` everywhere to refer to experiment keys.

With the name `enabled` no longer being used for a list of enabled keys,
this patch uses `enabled` instead of `ok`, resulting in a clearer
`if enabled { ... }`.
Copy link
Contributor

@yob yob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the great PR description, I learnt some things.

The changes looks reasonable 👍

@pda pda merged commit 85967dd into master Aug 20, 2020
@pda pda deleted the refactor-experiments-local-var-naming branch August 20, 2020 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants