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'm creating tool to add new virtualhost on UNIX-box. One of tasks is to add a new user and database to MariaDB (aka MySQL) server. In fact - this my first 'real' attempt to use OOP.

Here is class, which I created for it:

class mysqlUserDb:
        warnings.filterwarnings('error')
        def __init__(self):
                self.root = 'root'
                self.host = 'localhost'
                self.rootpw = 'p@ss'

                try:
                        print '\nChecking MySQL connection...'
                        self.db = MySQLdb.connect(self.host, self.root, self.rootpw)
                        self.cursor = self.db.cursor()
                        self.cursor.execute('select version()')
                        print 'Connection OK, proceeding.'
                except MySQLdb.Error as error:
                        print 'Error: %s ' %error + '\nStop.\n'
                        sys.exit()

        def createdb(self, db):
                print '\nCreating database...'
                try:
                        self.cursor.execute('create database if not exists ' + db)
                        self.cursor.execute('show databases like ' + '\'' + db + '\'')
                        dbs = self.cursor.fetchone()
                        print 'Database created: %s' %dbs
                except Warning as warn:
                        print 'Warning: %s ' %warn  + '\nStop.\n'
                        sys.exit()

        def grants(self, user, userpass, db):
                print '\nGrant privilegies... '
                try:
                        self.cursor.execute('grant all on ' + db + '.*' + 'to ' + '\'' + user + '\'' + "@'localhost'" + 'identified by ' + '\'' + userpass + '\'')
                        self.cursor.execute('select user, db from mysql.db where db=' + '\'' + db + '\'')
                        grs = self.cursor.fetchall()
                        print 'Access granted: %r' %grs
                except Warning as warn:
                        print 'Warning: %s ' %warn  + '\nStop.\n'
                        sys.exit()
                except MySQLdb.Error as error:
                        print 'Error: %s ' %error + '\nStop.\n'
                        sys.exit()

        def __del__(self):
                print '\nFinishing operations...'
                self.cursor.close()
                self.db.close()
                print 'Done.\n'

Next, in tool it call like:

import createvhostFunctsClass as fun

mysql = fun.mysqlUserDb()
mysql.createdb('db_2')
mysql.grants('user', 'pass', 'db_2')
del mysql

(it's just example, not included yet in tool).

And its works like:

$ ./myclass.py

Checking MySQL cconnection...
Connection OK, proceeding.

Creating database...
Database created: db_2

Grant privilegies...
Access granted: ('user', 'db_2')

Finishing operations...
Done.

I'm pretty sure, here is lot of stuff, which I missed or did incorrectly (or not the "Python way").

Also, I'm not sure - which way better:

  1. create methods for each action (add database, add user) and just call this methods from script (currently done)
  2. create one method, which will get needed action ('create database lalala') as argument and call this method several times in script
share|improve this question

2 Answers 2

up vote 7 down vote accepted

(This is in addition to what @200_success already said about exit codes and error handling.)

Your main questions

I'm pretty sure, here is lot of stuff, which I missed or did incorrectly (or not the "Python way").

The convention is to use CamelCase for class names. So MysqlUserDb would be better for your class. See PEP8 for more details.

Don't mix string concatenation and formatting expressions like this:

print 'Error: %s ' % error + '\nStop.\n'

Stick to formatting expressions without concatenation:

print 'Error: %s\nStop.\n' % error

You do this a lot. Apply this technique everywhere.


Similarly, you're not using formatting expressions enough. For example in SQL queries like this one:

self.cursor.execute('show databases like ' + '\'' + db + '\'')

With all those quotations and escaping, it's easy to overlook a mistake. It becomes simpler if you use formatting expression, and if you change the outermost quotes to double quotes:

self.cursor.execute("show databases like '%s'" % db)

Your code appears to be for Python 2.x. To be a bit more ready for a possible future migration to Python 3.x, I recommend to start writing your print ... statements as print(...), for example:

print('Connection OK, proceeding.')

Also, I'm not sure - which way better:

  1. create methods for each action (add database, add user) and just call this methods from script (currently done)
  2. create one method, which will get needed action ('create database lalala') as argument and call this method several times in script

Your current approach is good: use a separate method for each separate action.

Unused fields

In __init__ you are setting these fields:

self.root = 'root'
self.host = 'localhost'
self.rootpw = 'p@ss'

But then you're not using them anywhere else within the class. So it seems these should not be fields, but perhaps global constants declared at the top of the class. I would also name them a bit differently:

DBROOTUSER = 'root'
DBROOTPASS = 'p@ss'
DBHOST = 'localhost'

Security

Are you sure you want to store the db's root account password in this script? Make sure the file has restricted permissions (do a chmod 0600 on it).

Needless to say, but better make sure the method parameters are cleaned before reaching your class to avoid SQL injection attacks. Or implement validation inside your methods.

Naming

Instead of these parameter names:

def createdb(self, db):
    # ...

def grants(self, user, userpass, db):
    # ...

I recommend this:

def createdb(self, dbname):
    # ...

def grants(self, dbuser, dbpass, dbname):
    # ...

This may be a matter of taste though.

share|improve this answer
    
Thanks a lot. Can't vote up, unfortunately. –  setevoy Sep 2 at 19:36
    
could you pelase explain - what you mean here: "Needless to say, but better make sure the method parameters are cleaned before reaching your class to avoid SQL injection attacks. Or implement validation inside your methods."? P.S. Tool designed for my use only on my only server, that's why I set root pass directly in script (and yes - I'ts owned by OS root too with 0700). Otherway - I'd prefer to set getpass.getpass() and ask pass everytime. –  setevoy Sep 3 at 17:19
    
As long as it's like that, it's fine. Keep it carefully secured. –  janos Sep 3 at 20:38

The error handling seems to be problematic.

By convention, a program should exit with a non-zero status to indicate that an error has occurred. By calling sys.exit(), you are indicating that the command completed successfully. Whatever process is calling your script will have a hard time determining whether the database operations succeeded or failed.

Also, warnings are attention-deserving problems that are non-fatal. Your script treats warnings as errors (which are fatal). Either change the vocabulary (tell the user that an error has occurred, not a warning, and exit with a non-zero status) or change the behaviour (print a warning to the user, but continue processing).

share|improve this answer
    
Thanks, @200. About sys.exit() - yes, will fix. About warnings - MySQL generates 'warning', if database exist and can't be created. Thus - I used 'warning' exceptions as 'erros' to rise error. Also - program can't be continued, if user add database, which currently present. IMHO. –  setevoy Sep 2 at 17:27
    
It's fine to treat warnings from MySQL as errors. Just make sure that what your script reports matches its behaviour. –  200_success Sep 2 at 17:29
1  
I assume in Python, exceptions can propagate up the call stack...right? Would it be better to just let them do so (or throw another exception with a more informative message), and have the caller decide what to do about the problem? (In a decent-sized app, for example, it might want to log the failure rather than just printing to stdout. I'd think it'd have trouble doing so if you just kill the app.) –  cHao Sep 2 at 21:40

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.