filter
Is useless here. It's purpose is to filter out values based on a predicate:
list(filter(f, data))
is the same as:
[d for d in data if f(d)]
Given that, in your case, f
is lambda p: (p[0], p[1])
, it will always evaluate to True
in a boolean context. Thus never filtering anything. I’m not sure what it was you were trying to achieve, but you can remove filter
without changing the behaviour:
def get_block_provider(self, provider_lookup):
for provider_key in self.__block_providers:
if provider_lookup in provider_key:
print("we've found the key: {key}".format(key=provider_key))
return self.__block_providers[provider_key]
return None
get_block_provider
I believe this method is to be used in the same kind of way than __getitem__
on dict
s. Thus you need to act accordingly; it's the principle of least attonishment: you claim to be somewhat a dict
then you get to act like a dict
.
First of, remove that print
. There is nothing justifying that line in a getter. If the user want to be sure it got something, it's its job to print something. It's not your classe's job.
Second, it might be better to raise a KeyError
as a dict
would have instead of returning None
:
def get_block_provider(self, provider_lookup):
for provider_key in self.__block_providers:
if provider_lookup in provider_key:
return self.__block_providers[provider_key]
raise KeyError("No block corresponding to lookup '{}'".format(provider_lookup))
Be a dict
You said that
It seemed that making multiple dictionaries or multiple entries in a single dictionary pointing to the same value object (ref type) was more maintenance than I wanted to commit to.
But I disagree. Especially since looking at the code and your comments, you’re only expecting 2 different kind of lookups for your blocks: ID and name.
You do not have to entirely expose a dict
interface. But you should use its strength. Your for
loop in get_block_provider
is a complete waste of time: you’re performing an \$O(n)\$ lookup where a dict
would have provided an \$O(1)\$ one.
Instead, your lookup should rely on the underlying dictionary one. And you should focus on having only one entry point to update both keys at once:
def register_block_provider(self, provider):
block = provider()
blocks = self.__block_providers
# update both keys at once
blocks[block.block_id] = blocks[block.name] = block
def get_block_provider(self, provider_lookup):
return self.__block_providers[provider_lookup]
You can even define
__getitem__ = get_block_provider
to mimic a dict
interface.
dict
s are for and good at, so you should use them that way unless you have a very good reason not to. \$\endgroup\$ – David Foerster Feb 13 '16 at 13:42dict
(a subclass ofcollections.UserDict
would be idiomatic) to hide the logic of adding and removing multiple keys at the same time. But this is leading off topic and would be much better on Stack Overflow or Software Engineering. \$\endgroup\$ – David Foerster Feb 13 '16 at 14:43