Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

At work, we have a "pair programming ladder" where you can keep track of who is pairing with whom (based on check-ins). The idea is to promote "promiscuous pairing" where each developer eventually pairs with every other developer. Anyways, in our team this was being updated manually, so I thought of writing a script that will parse the logs and generate a table.

Given these are the names of the developers: "Adam", "Bill", "Carl", "David", "Eddie", "Frank", "George"

The output would look like so:

       |Adam   
Bill   |0      |Bill  
Carl   |0      |2      |Carl  
David  |1      |0      |0      |David  
Earl   |0      |0      |3      |0      |Earl 
Frank  |0      |0      |0      |2      |1      |Frank    
George |0      |0      |0      |0      |2      |0      |George 
Solo   |1      |0      |0      |0      |0      |1      |0      |Solo
Other  |1      |0      |0      |0      |0      |2      |1      |0      |Other

This is my script:

from git import *
import re
from collections import Counter

list = ["Adam", "Bill", "Carl", "David", "Eddie", "Frank", "George"]

def build():
    repo = Repo("/home/developer/work/commerce-git/commerce-project")
    master = repo.heads.master
    pairsDict = {}
    for commit in repo.iter_commits(master, since='2013-02-06+14:00', reverse=True):
        for person in list:
            if person not in pairsDict:
                pairsDict[person] = Counter()
            if person in commit.message:
                pairedWith = ""

                m = re.search('(\w+)/(\w+)\s', commit.message)
                if not m:
                    pairedWith = "Solo"
                else:
                    if m.group(1) == person:
                        pairedWith = m.group(2) #pair is right of slash
                    else:
                        pairedWith = m.group(1) #pair is left of slash
                    if pairedWith not in list:
                        pairedWith = "Other"
                pairsDict[person][pairedWith] += 1
                if pairedWith not in pairsDict:
                    pairsDict[pairedWith] = Counter()
                pairsDict[pairedWith][person] += 1
                break;

    return pairsDict

def print_table():
    pairsList = list + ["Solo", "Other"]
    pairsDict = build()
    str = "" 
    for index,person in enumerate(pairsList):
        if (index == 0):
            str += '|'.rjust(8) + person.ljust(7) + '\n'
        else:
            str += person.ljust(7) + '|'
            for paired in pairsList[:index]:
                count  = pairsDict[person][paired]
                str += '{:<7}|'.format(count)
            str +=  person.ljust(7)+'\n'
    print str
    return str

if __name__ == "__main__":
    print_table();

The scripts parses the git logs using GitPython, where a commit message is expected to look like this: "Adam/David - Refactored app to be more polyglot-y"
From there it constructs a HashMap of Counters to keep track of who has paired with whom.
I'm going to eventually feed this data structure into a simple Django web-app so people can view it in a browser.

Do you have any suggestions for this script? Could it be more "pythonic"? Is there anything I could do to refactor it to be simpler to read, or better performance-wise?

share|improve this question
add comment

2 Answers 2

up vote 4 down vote accepted

1. Comments on your code

  1. No docstrings! What do these functions do and what kind of values do they return?

  2. Python statements don't need to be terminated with a semicolon: they finish at the end of a line.

  3. Is it really necessary to do from git import * given that you only actually use git.Repo?

  4. There's a built-in function called list, so it's a bad idea to use that as the name of a variable. Also, list is a vague name: list of what? I suggest something like users.

  5. Similarly, build is a vague name. Build what? I suggest something descriptive like count_commit_pairings.

  6. It's conventional (following PEP8) for Python variables to use lower case with underscores. If you plan to collaborate with other Python programmers, use variable names like pairs_dict and paired_with instead of pairsDict and pairedWith.

  7. Constants like '2013-02-06+14:00' shouldn't be buried deep in the code, but should be pulled out to become global variables or function parameters.

  8. You could avoid having to write these lines:

    if person not in pairsDict:
        pairsDict[person] = Counter()
    

    and these lines:

    if pairedWith not in pairsDict:
        pairsDict[pairedWith] = Counter()
    

    if you made pairsDict into a defaultdict(Counter). See collections.defaultdict.

  9. Since you're going to be looking up users to see whether they need to be replaced with 'Other', it's good practice to convert the list of users into a set, so that looking up a name can be done in constant time.

  10. It's wasteful to iterate over the users and try to match each one against the commit message. Why not do a regular expression match against the commit message just once and then figure out which users are mentioned?

  11. You use the line

    if person in commit.message:
    

    to determine if person is the committer. But what if someone makes a commit without writing their name in the commit message (false negative)? What if someone's name appears in the commit message by coincidence (false positive)? It seems to me that looking at commit.committer.name would be more reliable.

  12. Backslash is an escape character in Python strings, so you should generally use the r modifier in front of strings where you want to preserve the backslashes. So write:

    m = re.search(r'(\w+)/(\w+)\s', commit.message)
    
  13. It's better to use the regular expression \b (word boundary) rather than \s (whitespace) to detect the end of a word. This is because \b matches if the word is followed by punctuation, or by the end of the string, whereas \s would fail in those cases.

  14. If the name of the pairing is supposed to be at the start of the commit message, you should use re.match to ensure this. But if it is allowed to be anywhere in the commit message you should write:

    m = re.search(r'\b(\w+)/(\w+)\b', commit.message)
    

    to ensure that the match starts at the beginning of a word.

  15. Each time you call re.search it has to compile the regular expression. Since you're going to do this for each commit, it makes sense to compile the regular expression just once by calling re.compile.

  16. In the section of code

    if m.group(1) == person:
        pairedWith = m.group(2) #pair is right of slash
    else:
        pairedWith = m.group(1) #pair is left of slash
    

    you assume that if person is in the message, and not to the left of the slash, then they must be to the right of the slash. But that doesn't follow at all. Suppose Gabrielle commits a change with the message

    Gabrielle/Frank -- fixed bug 123 found by Alice
    

    it looks to me as though your code will incorrectly deduce that this is a change committed by Alice and Frank. So I think it would be worth taking more care to detect (and perhaps report?) errors in the commit messages. People make mistakes!

2. Revised build function

Addressing all the issues discussed above yields the following code. But beware: I haven't tested this, so most likely there are mistakes.

from git import Repo
import re
from collections import Counter, defaultdict
import sys

def count_commit_pairings(repository = '/home/developer/work/commerce-git/commerce-project',
                          since = '2013-02-06+14:00',
                          users = 'Alice Bill Clare David Emma Frank Gabrielle'.split()):
    """
    Count the number of times each pair of users has made a commit.

    `repository` is the git repository to examine.
    `since` is the date at which to start looking at the commits.
    `users` is a list of users to count commits for.

    Return the counts in the form of a dictionary mapping user name to
    a dictionary mapping partner name to count of commits by that
    pair. Commits not made by a pair are filed as if by the committer
    and the fake user 'Solo'. User names not found in `users` are
    replaced by 'Other'.
    """
    repo = Repo(repository)
    pairs = defaultdict(Counter)
    users = set(users)
    pair_re = re.compile(r'(\w+)/(\w+)\b')

    def username(name):
        """Return the username to use for `name`."""
        return name if name in users else 'Other'

    def partner(commit):
        """Return the partner of the committer, or 'Solo' if none found."""
        m = pair_re.match(commit.message)
        if not m:
            return 'Solo'
        if commit.committer.name not in m.groups():
            sys.stderr.write("Bad commit {0.hexsha}: committer {0.committer.name} "
                             "missing from pair {1}."
                             .format(commit, m.group(0)))
            return username(m.group(1))
        partner = set(m.groups()).difference([commit.committer.name])
        if len(partner) != 1:
            sys.stderr.write("Bad commit {0.hexsha}: duplicate pair {1}."
                             .format(commit, m.group(0)))
            return 'Solo'
        return username(partner.pop())

    for commit in repo.iter_commits(repo.heads.master, since=since, reverse=True):
        u1 = username(commit.committer.name)
        u2 = partner(commit)
        pairs[u1][u2] += 1
        pairs[u2][u1] += 1

    return pairs

(This seems like plenty for one answer. Maybe some other person can look at print_table.)

share|improve this answer
    
Regarding print_table, that was more of a 'test method' for me, so I'm not as concerned about that. I've taken suggestions from both answers, but your answer is more thorough and simplified my code a lot. Thanks! –  Atif Feb 18 '13 at 21:36
add comment
from git import *

Generally import from * is frowned upon as it makes it more difficult to figure out where stuff comes from

import re
from collections import Counter

list = ["Adam", "Bill", "Carl", "David", "Eddie", "Frank", "George"]

Don't names things after types. Global constants should also be in ALL_CAPS. So this should probably be DEVELOPERS = [...]

def build():
    repo = Repo("/home/developer/work/commerce-git/commerce-project")

Putting an absolute path to the repository is probably not all that great. It makes more assumptions about the setup of the machine than we'd like.

    master = repo.heads.master
    pairsDict = {}

Python convention is lowercase_with_underscores for local names

    for commit in repo.iter_commits(master, since='2013-02-06+14:00', reverse=True):
        for person in list:
            if person not in pairsDict:
                pairsDict[person] = Counter()

Use pairsDict = collections.defaultdict(Counter) so you don't need to do this. Alternately use a two element tuple as the key.

            if person in commit.message:

Shouldn't you do something like look at the username recorded by git as to who did the comment rather then checking the commit message?

                pairedWith = ""

Don't assign variables with meaningless content if you are just going to reassign it later

                m = re.search('(\w+)/(\w+)\s', commit.message)

This is an okay use of a single letter variable name, but I'd still call it match.

                if not m:
                    pairedWith = "Solo"
                else:
                    if m.group(1) == person:
                        pairedWith = m.group(2) #pair is right of slash
                    else:
                        pairedWith = m.group(1) #pair is left of slash

This would be simpler if you didn't iterate over the list the developers. Just figure out who the developers were by regexing the message.

                    if pairedWith not in list:
                        pairedWith = "Other"
                pairsDict[person][pairedWith] += 1
                if pairedWith not in pairsDict:
                    pairsDict[pairedWith] = Counter()
                pairsDict[pairedWith][person] += 1
                break;

    return pairsDict

def print_table():
    pairsList = list + ["Solo", "Other"]
    pairsDict = build()
    str = "" 
    for index,person in enumerate(pairsList):
        if (index == 0):
            str += '|'.rjust(8) + person.ljust(7) + '\n'

Don't build up strings by adding, that's inefficent. You are better off just printing here

        else:
            str += person.ljust(7) + '|'
            for paired in pairsList[:index]:
                count  = pairsDict[person][paired]
                str += '{:<7}|'.format(count)
            str +=  person.ljust(7)+'\n'
    print str
    return str

Don't both return a print. Either return the string and let the caller print or print the string and return nothing.

if __name__ == "__main__":
    print_table();
share|improve this answer
    
Thanks for the review! As for "Shouldn't you do something like look at the username recorded by git as to who did the comment rather then checking the commit message?" I want to the filter the commit messages from my team specifically, as other teams commit to the same branch. –  Atif Feb 18 '13 at 19:31
2  
@Atif, but it would still seem less error prone to check the username against a list your team's usernames –  Winston Ewert Feb 18 '13 at 19:58
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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