Today I was viewing my colleague's code and I saw a function like this:
def manager_skill_tree_func(*args, **kwargs):
"""# manage_skill_tree: Initialize the manage skill tree
"""
skill_tree0 = {}
head = []
tree_skill_list = {}
manage_skill = globals()['manage_skill']
for k, v in manage_skill.iteritems():
if not k in skill_tree0:
skill_tree0[k] = {}
if v['tree'] not in tree_skill_list:
tree_skill_list[v['tree']] = set([k])
else:
tree_skill_list[v['tree']].add(k)
pre_k_list = v['pre_skill']
if not pre_k_list:
head.append(k)
for pre_k in pre_k_list:
pre_k = int(pre_k)
if not pre_k in skill_tree0:
skill_tree0[pre_k] = {}
skill_tree0[pre_k][k] = skill_tree0[k]
# Rest of the function is omitted here
There are lots of readability issues in this piece of code, but my attentions was mostly draw by the for k, v in manage_skill.itermitems()
statement. This kind of statement was heavily used in the project, it seems that my colleague doesn't like to specify what kind of keys and values they are obtaining from the dict
.
For me, I'll use something like for index, arg_dict in dict_sample.iteritems()
if the key is a number and the value is another dict, and for employee_tuple in employee_info
to specify the item type I'll get tuple from the list.
I think names should offer some meaning, in the statement for pre_k in pre_k_list:
of above code, I got no information about what kind of data this pre_k
is, except that it's not called k
again.
I would use for k, v in sample_dict.items()
or for i in sample_list
only when the structure was already mentioned and is really obvious, like:
import random
sample_dict = {}
for i in xrange(10):
sample_dict[i] = random.choice([1,2,3])
for k,v in sample_dict.items():
sample_dict[k] = k/v
What's your opinion on for k,v in unknown_structure_dict.iteritems()
?
PS: As an extra gift, here is a pylint evaluation of a large file for @jonrsharpe.
A weak defend for this disaster score:
- Project lasts for years
- Written by plenty of programmers(from novice to veteran)
- Lots of requirement changes, lots of hot-fix.
- Some modules or classes are loaded at running time.
Anyway, enjoy this:
+-----------------------------+------------+
|message id |occurrences |
+=============================+============+
|bad-whitespace |1572 |
+-----------------------------+------------+
|bad-continuation |479 |
+-----------------------------+------------+
|line-too-long |303 |
+-----------------------------+------------+
|unused-argument |99 |
+-----------------------------+------------+
|invalid-name |95 |
+-----------------------------+------------+
|no-member |65 |
+-----------------------------+------------+
|missing-docstring |32 |
+-----------------------------+------------+
|redefined-outer-name |31 |
+-----------------------------+------------+
|trailing-whitespace |14 |
+-----------------------------+------------+
|unused-variable |8 |
+-----------------------------+------------+
|global-variable-not-assigned |6 |
+-----------------------------+------------+
|fixme |6 |
+-----------------------------+------------+
|unnecessary-semicolon |5 |
+-----------------------------+------------+
|bare-except |3 |
+-----------------------------+------------+
|eval-used |2 |
+-----------------------------+------------+
|too-many-locals |1 |
+-----------------------------+------------+
|too-many-lines |1 |
+-----------------------------+------------+
|superfluous-parens |1 |
+-----------------------------+------------+
|multiple-statements |1 |
+-----------------------------+------------+
|empty-docstring |1 |
+-----------------------------+------------+
Global evaluation
-----------------
Your code has been rated at -47.74/10