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.

I haven't had anyone help me out with code review, etc, so I thought I'd post a Python class I put together for interfacing with Telnet to get information from a memcached server.

import re, telnetlib

class MemcachedStats:

    _client = None

    def __init__(self, host='localhost', port='11211'):
        self._host = host
        self._port = port

    @property
    def client(self):
        if self._client is None:
            self._client = telnetlib.Telnet(self._host, self._port)
        return self._client

    def key_details(self, sort=True):
        ' Return a list of tuples containing keys and details '
        keys = []
        slab_ids = self.slab_ids()
        for id in slab_ids:
            self.client.write("stats cachedump %s 100\n" % id)
            response = self.client.read_until('END')
            keys.extend(re.findall('ITEM (.*) \[(.*); (.*)\]', response))
        if sort:
            return sorted(keys)
        return keys

    def keys(self, sort=True):
        ' Return a list of keys in use '
        return [key[0] for key in self.key_details(sort=sort)]

    def slab_ids(self):
        ' Return a list of slab ids in use '
        self.client.write("stats items\n")
        response = self.client.read_until('END')
        return re.findall('STAT items:(.*):number', response)

    def stats(self):
        ' Return a dict containing memcached stats '
        self.client.write("stats\n")
        response = self.client.read_until('END')
        return dict(re.findall("STAT (.*) (.*)\r", response))

This is also up on GitHub.

I would love some feedback on:

  • Organization
  • Better ways of accomplishing the same result
share|improve this question
2  
It has been decided on meta that questions should not just link to the code, but at least include the most relevant pieces of code in the question itself. So I've took the liberty of editing your code into the question. –  sepp2k Feb 6 '11 at 5:14
    

1 Answer 1

up vote 5 down vote accepted

The pattern

self.client.write("some command\n")
response = self.client.read_until('END')

appears three times in your code. I think this is often enough to warrant refactoring it into its own method like this:

def command(self, cmd):
    self.client.write("%s\n" % cmd)
    return self.client.read_until('END')

In key_details you're using extend to build up a list. However it's more pythonic to use list comprehensions than building up a list imperatively. Thus I'd recommend using the following list comprehension:

regex = 'ITEM (.*) \[(.*); (.*)\]'
cmd = "stats cachedump %s 100"
keys = [key for id in slab_ids for key in re.findall(regex, command(cmd % id))]

Afterwards you do this:

if sort:
    return sorted(keys)
return keys

Now this might be a matter of opinion, but I'd rather write this using an else:

if sort:
    return sorted(keys)
else:
    return keys

I think this is optically more pleasing as both returns are indented at the same level. It also makes it immediately obviously that the second return is what happens if the if condition is false.

share|improve this answer
    
thanks! I've incorporated some of these into the code on github. I also added precompiled versions of the regex into the class –  dlrust Feb 7 '11 at 5:24
    
Also, as far as the list comprehension goes. Since re.findall(regex, command(cmd % id)) can return multiple items, I am using extend. Is there a way to build a list that extends itself vs item by item building? –  dlrust Feb 7 '11 at 5:26
    
@dlrust: Yes, of course, you need to add another for in the list comprehension to get a flat list. I actually meant to do that, but forgot somehow. –  sepp2k Feb 7 '11 at 5:31
    
Thanks. Not sure if this is the best way to do this, but it works keys = [j for i in [self._key_regex.findall(self.command(cmd % id)) for id in self.slab_ids()] for j in i] –  dlrust Feb 7 '11 at 19:39
    
@lrust: By using an inner list like that, you need one more for than you would otherwise. Does [key for id in slab_ids for key in self._key_regex.findall(self.command(cmd % id))] not work for you? –  sepp2k Feb 7 '11 at 19:43

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.