10
\$\begingroup\$

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
\$\endgroup\$
0
7
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ thanks! I've incorporated some of these into the code on github. I also added precompiled versions of the regex into the class \$\endgroup\$ – dlrust Feb 7 '11 at 5:24
  • \$\begingroup\$ 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? \$\endgroup\$ – dlrust Feb 7 '11 at 5:26
  • \$\begingroup\$ @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. \$\endgroup\$ – sepp2k Feb 7 '11 at 5:31
  • \$\begingroup\$ 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] \$\endgroup\$ – dlrust Feb 7 '11 at 19:39
  • \$\begingroup\$ @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? \$\endgroup\$ – sepp2k Feb 7 '11 at 19:43

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.