in/[] antipattern pervasive in code.

Issue #7 new
TomRitchford
created an issue

Sorry for all the bug reports, but since I was here...

There's an awful lot of code that looks like this:

# Actual example:
if data_type in self.yaml_multi_representers:
  node = self.yaml_multi_representers[data_type](self, data)

if key in dictionary:
  process(dictionary[key])

This is a known anti-pattern in Python, because you incur the cost of the lookup in the hash table twice. The get method is very useful for this - so do consider the following alternatives.

# If you know that None is never a valid dictionary entry.

value = dictionary.get(key)
if value is not None:
  process(value)

# If the dictionary might contain None.

NONE = object()  # NONE can be a global to avoid constructing new objects each time.

value = dictionary.get(key, NONE)

if value is not NONE:
  process(value)

Comments (2)

  1. Nicholas Chammas

    I can't comment on how big of a performance impact your proposed changes would have, but for the record I find the first block of code much more readable and Pythonic. I would not call it an "anti-pattern" by any stretch, and I would not count this as a bug unless the performance impact of the lookups was really big.

    Regarding performance, I would guess that people using YAML are more concerned about compatibility and user-friendliness than performance (assuming performance is not appalling), but that's just a guess.

  2. TomRitchford reporter

    This was more of a quibble than a big bug but I still feel it's a small deficiency. Dictionary lookups aren't super-expensive, but they aren't super-super-cheap either. Small optimizations like that are probably pointless in user code, but in library code that's hopefully used by a lot of people, it's worth spending a marginal amount of time for marginal speedups.

    Overall, testing for existence in a table right before looking up in that table is an anti-pattern in any language, and some languages like C++ have constructs to deter you from doing that (though perhaps at too great a cost?)

    An edge case, regardless. Feel free to disregard!

  3. Log in to comment