1
\$\begingroup\$

I just need to know if it's decent or not.

Project home

import glob
import tornado
import tornado.httpserver
import tornado.ioloop
import tornado.options
import tornado.web
import tornado.websocket
class server(tornado.websocket.WebSocketHandler):
    def on_message(self, message):
        self.modDirectory = 'modules/*.py'
        self.files = glob.glob(self.modDirectory)
    #print(self.files)
        #CALL ON INIT
    self.modules = dict()
        self.commands = list()
    self.msg = dict()
    self.ip = self.request.remote_ip
    self.uip = {}
        self.loadModules()
        data = message.split(':', 1)
        if len(data) == 1:
            cmd, args = data[0]
        else:
            cmd, args = data
        key = cmd
        self.getCommand(cmd)
        cmd = self.getCommand(key)
        if cmd:
            if cmd:
                cmd.check(self, args)
    def loadModules(self):
        '''
        load modules that are found in self.files>glob.glob('directory/*.py')
        '''
    self.modules = {}
    self.commands = []
        for f in self.files:
        execfile(f, self.modules)
    for key, value in self.modules.items():
       if key == 'init':
        value(self)
    def callProtocal(self, evt):
        '''
        sends the protocals
        '''
    self.write_message(evt)
    def clearCommands(self):
        '''
        clears protocal list
        '''
        self.commands = list()
    def addCommand(self, *args, **kw):
        '''
        add the protocal to the list protocal>description>function>id
        '''
        co = Command(*args, **kw)
        self.commands.append(co)
    def getCommand(self, name):
        '''
        search for the protocal then returns the result
        '''
        name = name.lower()
        for command in self.commands:
            if command.name == name:
                return command
    return None
class Command(object):
        def __init__(self, name, desc, func, number = 0):
            self.name = name
            self.desc = desc
            self.number = int(number)
            self.func = func

        def run(self, mgr, args):

            if not args: return False
            else: return True
        def check(self, mgr, args):
            if not self.run(mgr, args): return
            else: self.func(mgr, args)
if __name__=="__main__":
    tornado.options.parse_command_line()
    app = tornado.web.Application(handlers=[(r'/', server)])
    __SERVER__ = tornado.httpserver.HTTPServer(app)
    __SERVER__.listen(address='localhost', port=6444)
    tornado.ioloop.IOLoop.instance().start()

mod.py

from db import database as self
from time import time
def init(mgr):
    mgr.addCommand('delMsg', 'delete a message', delMsg, 3)
    mgr.addCommand('delUserMsg', 'delete a users messages', delUserMsg, 4)
    mgr.addCommand('addMod', 'add a moderator', addMod, 5)
    mgr.addCommand('removeMod', 'remove a mod', removeMod, 6)
    mgr.addCommand('banUser', 'ban a user', banUser, 7)
    mgr.addCommand('unbanUser', 'unban a user', unBan, 8)

def delMsg(mgr, args):
    room = args[0]
    msgid = args[1]
        self.delMsg(room, msgid)
        mgr.callProtocal('msgremove:%s:%s:%s' % (room, msgid, time()))
def delUserMsg(mgr, args):
    room = args[0]
    user = args[1]
        self.delUserMsg(room, user)
        mgr.callProtocal('usermsgremove:%s:%s:%s' % (room, user, time()))
def addMod(mgr, args):
    room = args[0]
    user = args[1]
        self.addMod(room, user)
        mgr.callProtocal('mod:%s:%s:%s' % (room, user, time()))
def removeMod(mgr, args):
    room = args[0]
    user = args[1]
        self.removeMod(room, user)
        mgr.callProtocal('demod:%s:%s:%s' % (room, user, time()))
def banUser(mgr, args):
    room = args[0]
    user = args[1]
    target = args[2]
        self.banUser(room, user, target)
        mgr.callProtocal('banned:%s:%s:%s:%s' % (room, user, target, time()))
def unbanUser(mgr, args):
    room = args[0]
    user = args[1]
    target = args[2]
        self.unbanUser(room, user, target)
        mgr.callProtocal('unbanned:%s:%s:%s:%s' % (room, user, target, time()))

room.py

from db import database as self
import random
def init(mgr):
    mgr.addCommand('bmsg', 'send a message to the room', message, number=1)
    mgr.addCommand('bauth', 'login', login, number=2)
def message(mgr, args):
    if self.isConnected(args[1]):
        user = args[1]
        room = args[2]
    if self.getRoom(room) != False:
            msg = args[3]
            _ip = self.ip(self.uid(user))
            params = "b:%s:%s:%s:%s:%s" % (user, room, msg, _ip, self.uid(user))
            mgr.callProtocal(params + '<br />')
    else:
        mgr.callProtocal('NROOM:NO ROOM FOUND')
    else:
        mgr.callProtocal('NROOM:NO ROOM FOUND')
        mgr.callProtocal('NT_CONNECTED:%s' % ('YOU ARE NOT SIGNED IN'))
def login(mgr, args):
    status =args[0]
    if status == 'bauth':
        room = args[1]
        usern = args[2]
        password = args[3]
        st = self.check(usern, password)
        if st == 'ok':
            id = self.uid(usern)
            conip = mgr.ip
            self.setConnected(usern, mgr.ip, room)
            _ip = self.ip(id)
            mgr.callProtocal('LGN_OK:%s:%s:%s:%s' % (room, usern, id, _ip))
        elif st == 'denied':
            anonid = mgr.setSsid(ip=_ip, room=room) 
            user = 'anon%s' % anonid
            self.setConnected(user, mgr.ip, room)
            mgr.callProtocal('LGN_DENIED:%s:%s' % (user, _ip))
    if status == 'blogAnon' and not self.getRoom(room) == False:
        anonid = random.randrange(1000, 9000)
                self.setSsid(_ip, anonid)
                user = 'anon%i' % anonid
                self.setAnon(user ,_ip)
                mgr.callProtocal('anon_login:%s:%s' % (user, _ip))

database.py

import time
import mid
import uid
import hashlib
import sqlite3 as m

global db

dbType = "sqlite"
if dbType == "sqlite": 
    db = m.connect('CHAT.sqlite3')
elif dbType == "mysql":
    import MySQLdb as sql
    host = ""
    user = ""
    password = ""
    dbname = ""
    db = sql.connect(host, user, password, dbname)

cur = db.cursor()

def ip(id, ip, mode='id'):
    try:
        cur.execute('select * from ip where id like("%s")' % (id))
        ip = ''.join([i[1] for i in cur.fetchall()])
        return ip
    except:
                ssid = ip.replace('anon', '')
                cur.execute('select * from ssid where like("%s")' % ssid)
                ip = ''.join([i[2] for i in cur.fetchall()])
                return ip

def setIp(id, ip):
    try:
        cur.execute('delete from ip where id like("%s")' %(id))
        cur.execute('insert into ip(id, ip) values("%s", "%s")' % (id, ip))
        db.commit()
    except:
        cur.execute('insert into ip(id, ip) values("%s", "%s")' % (id, ip))
                db.commit()
def setSsid(room, ip):
    sid = random.randrange(1000, 9000)
    try:
        cur.execute('select room, ssid from ssid room="%s", ssid="%s"' % (room, str(sid)))
        ssid = [ssid[1] for ssid in cur.fetchall()]
        if str(sid) in ssid:
            while True:
                r = random.randrange(1000, 9000)
                if r not in ssid:
                    cur.execute('insert into ssid(ssid, room, ip) values("%s", "%s", "%s")' % (r, room, ip))
                            db.commit()
                    return r
                    break
    except:
        cur.execute('insert into ssid(ssid, room, ip) values("%s", "%s", "%s")' % (r, room, ip))
                db.commit()
        return str(sid)

def setConnected(user, ip, room):
    try:
                cur.execute('delete from connected where room="%s", user="%s"' %(room, user))
                cur.execute('insert into connected(user, ip) values("%s", "%s", "%s")' % (user, ip, room))
                db.commit()
        except:
                cur.execute('insert into connected(user, ip) values("%s", "%s", "%s")' % (user, ip, room))
                db.commit()
def getConnected(room, user):
    try:
         cur.execute('select room, user from connected where room="%s", user="%s"' % (room, user))
                 user = ''.join([i[0] for i in cur.fetchall()])
                 return True
    except: return False
def setMessage(room, user, msg):
        userid = UID(user)
        ip = ip(userid)
    msgid = mid.generate()
    cur.execute('insert into messages(room, user, message, msgid, userid, ip) values("%s", "%s", "%s", "%s", "%s", "%s")' % (room, user, msg, userid, ip))
        db.commit()
def getMessage(user):
        cur.execute('select room, user from messages user="%s"' % user)
        lis = [a[0:] for a in cur.fetchall()]
        return lis
def delMsg(room, msgid):
        cur.execute('delete from messages where room="%s" msgid="%s"' %(room, msgid))
        db.commit()
def delUserMsg(room, user):
        cur.execute('delete from messages where room="%s", user="%s"' %(room, msgid))
        db.commit()

def banUser(room, user, target):
        tid = UID(target)
    try:
        cur.execute('delete from banlist where room="%s", user="%s", target="%s", id="%s", ip="%s"' % (room, user, target, tid, ip))
                cur.execute('insert into banlist(room, user, target, tid, ip, time) values("%s", "%s", "%s", "%s", "%s", "%s")' % (user, target, tid, ip, time.time()))
                db.commit()
        except:
        cur.execute('insert into banlist(room, user, target, tid, ip, time) values("%s", "%s", "%s", "%s", "%s", "%s")' % (user, target, tid, ip, time.time()))
                db.commit()
def unbanUser(room, user, target):
        if UID(target) == 'ip':
                ip = ip(user, mode = 'user')
    try:
        cur.execute('delete from banlist where room="%s", user="%s", target="%s", id="%s", ip="%s"' % (room, user, target, tid, ip))
        db.commit()
    except: pass
def getRoomOwner(room):
        cur.execute('select * from rooms where room="%s"' % room)
        owner = ''.join([n[1] for n in cur.fetchall()])
        return owner
def getBanlist(room):
    cur.execute('select * from banlist room="%s"' % room)
    l = [a[1] for a in cur.fetchall()]
    return l
def addMod(room, user):
     try:
                cur.execute('delete from mods where room="%s", user="%s"' % (room, user))
        cur.execute('insert into mods(room, user, userid) values("%s", "%s", "%s")' % (room, user, time.time()))
                db.commit()
         except:
                cur.execute('insert into mods(room, user, userid) values("%s", "%s", "%s")' % (room, user, time.time()))
                db.commit()
def removeMod(room, user):
     try:
        cur.execute('delete from mods where room="%s", user="%s"' % (room, user))
        db.commit()
     except:
        return False

def register(user, password):
    cur.execute('insert into register(user, password userid, time) values("%s", "%s", "%s")' % (user, hashlib(password).hexdigest(),  uid.generate(), time.time()))
    db.commit()
def UID(user):
    cur.execute('select * from register where user like("%s")' % user)
    Uid = ''.join([n[2] for n in cur.fetchall()])
    return Uid
def check(user, password):
    cur.execute('select * from register where user like("%s")' % user)
    auth = ''.join([n[1] for n in cur.fetchall()])
    if hashlib(password).hexdigest == auth: return 'ok'
    else: return 'denied'
def getMods(room):
    cur.execute('select * from mods where room like("%s")' % mods)
    l = [i[1] for i in cur.fetchall()]
    return l
def getRoom(room, check = True):
    cur.execute('select * from rooms where room like("%s")' % room)
    r = ''.join([n[0] for n in cur.fechall()])
    if check == True:
        if r == room: return True
        elif r != room: return False
    elif check == False:
        return room
\$\endgroup\$
  • \$\begingroup\$ Please double-check your indentation. It looks like you have mingled tabs and spaces in your code, which is a bad idea, especially in Python — and now you can see why. \$\endgroup\$ – 200_success Sep 10 '14 at 4:27
3
\$\begingroup\$

I just need to know if it's decent or not.

Well, it's not.

  • You're not following PEP8, the official Python style guide
  • You have spaces mixed with tabs, which can lead to nasty bugs that will really hurt you

The code is buggy and strange in many places.


def on_message(self, message):
    self.modDirectory = 'modules/*.py'
    self.files = glob.glob(self.modDirectory)

The variable is called modDirectory, but its value is a glob. That's misleading. But the bigger problem is that modDirectory is an instance variable, but it's not used anywhere else in the posted code. This is better:

self.files = glob.glob('modules/*.py')

There is a bug here:

data = message.split(':', 1)
if len(data) == 1:
    cmd, args = data[0]

This assignment won't work, you'll get ValueError: too many values to unpack (expected 2).


What is going on here?

self.getCommand(cmd)
cmd = self.getCommand(key)

First you call self.getCommand and throw away the result? It's not clear what you're doing here and why. If the first call is necessary, that would mean that self.getCommand has side effects, which is a bad practice in general, especially for something that looks like a getter.


What's going on here:

if cmd:
    if cmd:

At this point I stopped reviewing. This is just too messy and far from decent. I bet you could spot these things yourself if you give it a careful read!

Here's what you can do to improve this, very easily:

  1. Install pep8 (pip install pep8). It's a command line tool that verifies specified file or an entire directory tree for PEP8 violations. As much as possible, follow it and fix everything it tells you.

  2. Install pyflakes (pip install pyflakes). Another command line tool, that checks for common mistakes in Python coding practices. Use it the same way as the pep8 tool.

If you use these tools to clean up your code and post again a new question, that will have a LOT better chance of being decent!

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Now that you point it out. I did do some needless things. As for indents its just how it pasted. But I should start using py 3 so I stop being lazy with tab/spaces. Considering py 3 is strict on that. Now the if cmd I didn't notice until now. I should also point out the valueError you mentioned I just forgot to add quotes as the second param. \$\endgroup\$ – user3103366 Sep 12 '14 at 2:21
2
\$\begingroup\$

I can't really comment on the python, but I see a couple issues with your SQL.

Firstly, you should really avoid using SELECT * FROM XYZ, you should always try to just return the columns you need. It makes it easier to understand what is actually happening and can speed up queries, it's pretty much a win-win.

Secondly, SQL is typically written with keywords such as SELECT or DELETE in uppercase.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ My SQL knowledge is limited, but I do see what you mean in some cases. As for the caps I know its good practice to do them in caps, that's what company's look for. But considering this was a spare time project I thought it really didn't matter. I did put it on github so I should of fixed all this to make it look professional. \$\endgroup\$ – user3103366 Sep 12 '14 at 2:34
  • 2
    \$\begingroup\$ @user3103366 that's an odd attitude, particularly from someone who's submitting to code review. You don't try to write good code just because "that's what company's [sic] look for" or "to make it look professional". You do it because it makes it easier for you and others to read, because it helps make you a better programmer, and because it gives you the feeling that you've created something good. The standards still apply for personal projects; you should aspire to write clean, readable code all of the time. \$\endgroup\$ – jonrsharpe Sep 14 '14 at 7:46

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.