5

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:

  1. Project lasts for years
  2. Written by plenty of programmers(from novice to veteran)
  3. Lots of requirement changes, lots of hot-fix.
  4. 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
  • 5
    You're asking about the names k and v, right? Not about the for... .iteritems() construct? – RemcoGerlich Mar 16 '15 at 10:18
  • @RemcoGerlich, yes – Zen Mar 16 '15 at 10:31
  • 4
    Meaningful variable names, or no pizza. Bad programmer! Bad!! No pizza! – Mawg Mar 16 '15 at 10:49
  • 8
    This question comes dangerously close to being a rant in disguise ("I'm curious if other people feel like I do" and "___ sucks, am I right?" on What types of questions should I avoid asking?). At the least, I'd consider it a leading question. – 200_success Mar 16 '15 at 12:09
  • 2
    I think the only reasonable solution to this is to bring it up during code review "for k,v ... let's name this instead something more transparent like for id,name or something!??" – Brandin Mar 16 '15 at 14:16
13

I agree with you, and e.g. pylint would complain about those names too (albeit purely on a length basis).

for k, v in ... gives the reader no helpful information about what they should be expecting to get from the dictionary, which makes the subsequent code harder to follow. For example, it's only at if v['tree']... that you find out that the value is (presumably!) a dictionary. This requires you to look at the code where the dictionary is built to figure out what should be in it, and means you cannot just read and understand the function on its own (especially as the docstring is useless).

Even something as simple as:

for skill_name, skill_dict in manage_skill.iteritems():

would make the code clearer.

  • 16
    Note: if the method were a generic traversal method, then k, v would be pretty natural names. It all depends on context. In fact, I have the feeling that the method is doing too much, and a generic traversal method could be extracted from it. The fact that the OP sees stuff like "all over the code" seems to indicate that there is some underlying structure to the dicts that is replicated across many different functions, and could be extracted into a library of generic traversal functions. Or possibly, the dicts should actually be objects, responsible for managing their own traversal. – Jörg W Mittag Mar 16 '15 at 12:21
  • @JörgWMittag that's a very good point – jonrsharpe Mar 16 '15 at 12:22
  • 19 lines of code for one function the OP posted with a comment "rest of the function omitted here", that's a lot for one function, especially for a language as expressive as Python. I feel there's a SkillTree abstraction missing. – Jörg W Mittag Mar 16 '15 at 12:25
  • @jonrsharpe, Most of my colleagues don't use pylint. The code was a huge project write by different levels of programers in years. So as it doesn't broke. People let it go. I'll append a pylint evaluation of one large files(thousands of lines). – Zen Mar 16 '15 at 13:45
  • 2
    @Zen wow, I didn't know pylint scores could go negative! You might want to look into how you can (slowly!) move towards better code throughout the project, e.g. programmers.stackexchange.com/q/135651/110531 – jonrsharpe Mar 16 '15 at 13:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.