Any recommendations to improve this function?

Question

I am very new to working with SQL queries. Any suggestions to improve this bit of code: (by the way, I really don't care about sql security here; this is a bit of code that will be in a pyexe file connecting to a local sqlite file - so it doesnt make sense to worry about security of the query here).

def InitBars(QA = "GDP1POP1_20091224_gdp", QB = "1 pork", reset = False):
    global heights, values
    D, heights, values, max, = [], {}, {}, 0.0001

    if reset: GHolder.remove()

    Q = "SELECT wbcode, Year, "+QA+" FROM DB WHERE commodity='"+QB+"' and "+QA+" IS NOT 'NULL'"  

    for i in cursor.execute(Q): 
        D.append((str(i[0]) + str(i[1]), float(i[2])))
        if float(i[2]) > max: max = float(i[2])

    for (i, n) in D: heights[i] = 5.0 / max * n; values[i] = n
    Gui["YRBox_Slider"].set(0.0)  
    Gui["YRBox_Speed"].set(0.0)

---

after following the advices, this is what I got:

def InitBars(QA = "GDP1POP1_20091224_gdp", QB = "1 pork", reset = False):
    global heights, values; D, heights, values, max, = [], {}, {}, 0.0001
    if reset: GHolder.remove()

    Q = "SELECT wbcode||Year, %s FROM DB WHERE commodity='%s' and %s IS NOT 'NULL'" % (QA, QB, QA) 
    for a, b in cursor.execute(Q): 
        if float(b) > max: max = float(b)
        values[a] = float(b)

    for i in values: heights[i] = 5.0 / max * values[i]

    Gui["YRBox_Slider"].set(0.0); Gui["YRBox_Speed"].set(0.0)

Answer 1

You should check for SQL injection. Make sure that there's no SQL statement in QA. Also you should probably add slashes if it applies.

Answer 2

1. Use

   Q = "SELECT wbcode, Year, %s FROM DB WHERE commodity='%s' and %s IS NOT 'NULL'" % (QA, QB, QA)
   

instead:

Q = "SELECT wbcode, Year, "+QA+" FROM DB WHERE commodity='"+QB+"' and "+QA+" IS NOT 'NULL'"

1. Care about security (sql injection).
2. Look at any ORM (SqlAlchemy, for example). It makes things easy :)

Answer 3

If this is a one-off script where you totally trust all of the input data and you just need to get a job done, then fine.

If this is part of a system, and this is indicative of the kind of code in it, there are several problems:

1. Don't construct SQL queries by appending strings. You said that you don't care about security, but this is such a big problem and so easily solved, then really -- you should do it right all of the time

2. This function seems to use and manipulate global state. Again, if this is a small one-time use script, then go for it -- in systems that span just a few files, this becomes impossible to maintain.

3. Naming conventions --- not following any consistency in capitalization

4. Names of things are not helpful at all. QA, D, QB, -- QA and QB don't even seem to be the same kind of thing -- one is a field, and the other is a value.

5. All kinds of questionable things are uncommented -- why is max .0001? What the heck is GHolder? What could that loop be doing at the end? Really, the code should be clearer, but if not, throw the maintainer a bone.

Answer 4

• Use more descriptive variable names than QA and QB.

• Comment the code.

• Don't put multiple statements in the same line

• Try not to use globals. Use member variables instead.

• if QA and QB may come from user input, don't use them to build SQL queries