Des recommandations pour améliorer cette fonction?

Question

Je suis très nouveau à travailler avec des requêtes SQL. Toutes les suggestions pour améliorer ce peu de code: (en passant, je ne me soucie vraiment pas de la sécurité sql ici, c'est un peu de code qui sera dans un fichier pyexe se connectant à un fichier local sqlite - donc il ne fait pas sens de s'inquiéter de la sécurité de la requête ici).

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) 

---

après avoir suivi les conseils, ce que je suis:

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

S'il s'agit d'un script unique dans lequel vous faites totalement confiance à toutes les données d'entrée et vous avez juste besoin de faire un travail, alors ça va.

Si cela fait partie d'un système, ce qui est une indication du genre de code, il y a plusieurs problèmes:

1. Ne pas construire des requêtes SQL en ajoutant des chaînes. Vous avez dit que vous ne vous souciez pas de la sécurité, mais c'est un gros problème et si facilement résolu, alors vraiment - vous devriez le faire droit tout le temps

2. Cette fonction semble utiliser et manipuler l'état global . Encore une fois, si c'est un petit script à usage unique, alors allez-y - dans les systèmes qui ne couvrent que quelques fichiers, cela devient impossible à maintenir.

3. Conventions de dénomination --- ne suivent pas une cohérence dans la capitalisation

4. Les noms des choses ne sont pas utiles du tout. QA, D, QB, - QA et QB ne semblent même pas être le même genre de chose - l'un est un champ, et l'autre est une valeur.

5. Toutes sortes de choses douteuses sont décommentées - pourquoi est max .0001? Que diable est GHolder? Que pourrait faire cette boucle à la fin? Vraiment, le code devrait être plus clair, mais sinon, jetez un os au mainteneur.

Answer 2

Vous devriez vérifier pour l'injection SQL. Assurez-vous qu'il n'y a pas d'instruction SQL dans le contrôle qualité. Aussi, vous devriez probablement ajouter des barres obliques si cela s'applique.

Answer 3

1. Utilisez

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

à la place:

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

1. Soins de la sécurité (injection sql).
2. Regardez n'importe quel ORM (SqlAlchemy, par exemple). Cela rend les choses faciles :)

Answer 4

• Utiliser des noms de variables plus descriptives que QA et QB.

• Commentez le code.

• Ne pas mettre plusieurs déclarations dans la même ligne

• Essayez de ne pas utiliser globals. Utilisez plutôt les variables membres.

• si QA et QB peuvent provenir de l'entrée utilisateur, ne pas les utiliser pour construire des requêtes SQL