Tornado chat with SockJS and Redis

Question

I have Tornado chat with export to Redis. Other scripts will export messages from Redis to MongoDB in the time. It works, but how is it written?

def tornadoredis_client():
    client = tornadoredis.Client(*configuration.REDIS_CONFIG)
    client.connect()
    return client


class PlansqTornadoChat(SockJSConnection):
    users = dict()
    timezones = dict()

    def on_open(self, info):
        self.authenticated = False
        self.timezone = None
        self.client = tornadoredis_client()

    @tornado.gen.engine
    def on_message(self, msg):
        data = proto.json_decode(msg)

        if data['type'] == 'auth':
            if bool(data.get('sid', None)):
                try:
                    int(data['sid'])  # check that sid is number, not word
                    self.user_sid = data['sid']
                    self.users[self.user_sid] = self
                    self.user_timezone = yield tornado.gen.Task(self.client.hget, 'timezones', self.user_sid)
                    self.timezones[self.user_sid] = self.user_timezone
                    self.authenticated = True
                    self.send(proto.json_encode({"body": "You was authorized", 'type': 'alert'}))
                except Exception, e:
                    logging.info("bad user's sid: " + str(data['sid']))
            else:
                self.send(proto.json_encode({"body": "You did not authorize", 'type': 'alert'}))
                self.close()

        elif data['type'] == 'message':
            if all([x in data for x in ['type', 'recipient', 'body', 'sender']]) \
                    and data['sender'] == self.user_sid:
                recipient = data['recipient']

                try:
                    int(recipient)  # check that sid is number, not word
                except Exception, e:
                    logging.info('sid of recipient is not number')
                    return

                self.thread = "_".join([str(x) for x in sorted([int(self.user_sid), int(recipient)])])

                # Save to Redis
                data_to_send = {
                    'thread_id': self.thread,
                    'sender': self.user_sid,
                    'recipient': recipient,
                    'body': data['body'],
                    'datetime': (datetime.now() - datetime(1970, 1, 1)).total_seconds()
                }

                yield tornado.gen.Task(self.client.sadd, "thread:" + self.thread, proto.json_encode(data_to_send))
                yield tornado.gen.Task(self.client.sadd, 'new_threads:all', "thread:" + self.thread)

                # Send to Socket
                data_to_send['type'] = 'message'

                # if user is online
                if recipient in self.users:
                    data_to_send['datetime'] = utc_to_localtime(data_to_send['datetime'], self.timezones[recipient])
                    self.users[recipient].send(proto.json_encode(data_to_send))

                data_to_send['datetime'] = utc_to_localtime(data_to_send['datetime'], self.user_timezone)
                self.send(proto.json_encode(data_to_send))

            else:
                logging.info('Bad request: ' + str(data))
        else:
            logging.info('Bad request: ' + str(data))

    def on_close(self):
        del self.users[self.user_sid]
        del self.timezones[self.user_sid]
        self.client.disconnect()

My configuration in configuration of main application (Flask + MongoDB). There only Redis configuration (host and port).

The rest of the code:

if __name__ == '__main__':
    logging.getLogger().setLevel(logging.DEBUG)
    logging.getLogger().setLevel(logging.INFO)

    EchoRouter = SockJSRouter(PlansqTornadoChat, '/socket')

    app = web.Application(EchoRouter.urls)
    app.listen(configuration.TORNADO_PORT)
    ioloop.IOLoop.instance().start()

Answer 1

I have some notes on Pythonic style.

People usually instantiate an empty dict with {}, instead of actually calling dict(). Usually, the only reason you need to call dict is if you're passing some variables to create your dictionary.

There's no need to explicitly cast a value to a boolean, Python can implicitly run a boolean test on any type. So you can just do this:

if data.get('sid', None):

I don't know exactly what type data is, but assuming it's a dictionary, get will default to returning None, so there's no need to make it an explicit parameter of get.

You have a long block inside a try followed by a blanket except Exception, e. There's a few problems here. First, your try block should be as narrow as possible. You can use return to prematurely end functions from the except block if you need to. Speaking of that except block, you should ensure that it's only catching errors you specifically expect. In this case, presumably you want to catch ValueErrors from a bad data['sid'] string, as well as catching KeyError for user_sids that don't show up in the users dictionary. Blanket exceptions will just mask errors you're not expecting, instead of handling the ones you know can occur in normal execution.

Lastly, don't use except Exception, e syntax, it will lead to trouble and was removed in Python 3. It causes ambiguities when you want to catch multiple exception types, like except KeyError, ValueError, e:. How is Python supposed to interpret that? This is why you should use except (KeyError, ValueError) as e: or except (KeyError, ValueError):, as you don't have to store the result in a variable if you're not using it.