6
\$\begingroup\$

I made a little toy project last semester. I would like to get some insights about my code, particularly the part of handling the database. It is a CRUD kind of traditional API.

def fword(lon,lat,radius,ids):
    try:
        result = {}
        c, conn = connection()
        c.execute("SELECT * FROM wordlocation WHERE ((longitude-%s)*(longitude-%s)+(latitude-%s)*(latitude-%s))<%s",(lon,lon,lat,lat,radius))
        words = []
        for row in c.fetchall():
            t = {}
            if row[0] in ids:
                continue
            t["name"] = row[1]
            t["definition"] = row[4]
            t["id"] = row[0]
            t["lat"] = row[2]
            t["long"] = row[3]
            words.append(t)
        result["words"] = words
        j = json.dumps(result)
        c.close()
        conn.close()
        return j
    except Exception as e:
        logging.info(str(e))

def pword(lat,lng,word,definition):
    try:
        c, conn = connection()
        c.execute("INSERT INTO wordlocation (word,longitude,latitude,definition,lang) VALUES(%s,%s,%s,%s,%s)",
        (thwart(word),lat,lng,thwart(definition),'english'))
        conn.commit()
        c.close()
        conn.close()
        return "success!"
    except Exception as e:
        logging.info(str(e))

def cword(word, username):
    try:
        c, conn = connection()
        blank_word = " " + word
        c.execute("UPDATE userstat SET completed=CONCAT(IFNULL(completed,''), %s) , score=score+1 WHERE username=%s;",(blank_word, username))
        conn.commit()
        c.close()
        conn.close()
        return "successful collect"
    except Exception as e:
        logging.info(str(e))

def stat(username):
    try:
        c, conn = connection()
        c.execute("SELECT * FROM userstat WHERE username=%s", (username,))
        dict = {}
        u = c.fetchone()
        dict['username'] = username
        dict['words'] = str(u[1]).split()
        dict['score'] = u[2]
        return json.dumps(dict)
    except Exception as e:
        logging.info(str(e))

The backend logic:

@app.route("/getWord", methods=['GET'])
def getWord():
    if 'long' in request.args and 'lat' in request.args and 'radius' in request.args and 'ids' in request.args:
        try:
            longitude = float(request.args["long"])
            latitude = float(request.args["lat"])
            radius = float(request.args["radius"])
            ids = request.args.getlist("ids")
        except:
            return "Bad Request Parameters"
        try:
            allwords_json = fword(longitude,latitude,radius,ids)
            return allwords_json
        except Exception as e:
            logging.info(str(e))
    else:
        return "bad request"

@app.route("/postWord",methods=["GET","POST"])
def postWord():
    if 'lat' in request.args and 'long' in request.args and 'word' in request.args and 'definition' in request.args:
        try:
            lat = float(request.args['lat'])
            lng = float(request.args['long'])
            word = str(request.args['word'])
            definition = str(request.args['definition'])
            return pword(lat,lng,word,definition)
        except Exception as e:
            logging.info(str(e))
    else:
        return "badPost"

There are many other endpoints that is in a similar fashion. One issue I am aware of is the variable naming seems not readable.

\$\endgroup\$
  • \$\begingroup\$ "the variable naming seems not readable" - have you tried to do anything about that? It doesn't seem reasonable to ask people to spend their time suggesting more changes if you haven't bothered to action the ones you're aware of. \$\endgroup\$ – jonrsharpe Mar 7 '17 at 11:21
  • \$\begingroup\$ @jonrsharpe yes I will react to this issue when I get the chance during spring break. I am just gathering the ideas I should be working on during the spring break. \$\endgroup\$ – Bobby Mar 7 '17 at 18:59
1
\$\begingroup\$

Obligatory reference to the style guide. You have incorrect whitespace, for example, and (worse) it's inconsistent:

@app.route("/getWord", methods=['GET'])
                    # ^ yes
@app.route("/postWord",methods=["GET","POST"])
                     # ^ preferably not, but *definitely* be consistent

if 'long' in request.args and 'lat' in request.args and 'radius' in request.args and 'ids' in request.args:

Would be neater as:

if all(arg in request.args for arg in ['long', 'lat', 'radius', 'ids']):

This also allows you to extract a REQUIRED_ARGS constant if you wish.


Your exception handling is too broad. For example:

try:
    lat = float(request.args['lat'])
    lng = float(request.args['long'])
    word = str(request.args['word'])
    definition = str(request.args['definition'])
    return pword(lat,lng,word,definition)
except Exception as e:
    logging.info(str(e))

You should have as little as possible inside the try block, and avoid bare except.

What could actually go wrong inside the try block? E.g. for the first two lines, given that you've already checked for the keys, only a ValueError would get thrown. And as you have similarly broad error handling in pword, you'd only get an error out of that line if the logger itself failed.

Also note that there's a logging.exception that actually takes an error object and deals with the traceback correctly.


You note correctly that the naming is poor. It is also, again, inconsistent:

def fword(lon,lat,...
def pword(lat,lng,...

These parameters have the same meaning, so should have the same name (and ideally the same order). Given that you'd always expect the pair, you could even use a (latitude, longitude) tuple as e.g. a coords argument. Also:

def pword(...):
def postWord():

Finally you have a bunch of functions that all create a cursor and connection, and two of them are almost identical but for the query string and arguments. Look into whether you could reduce duplication by abstracting those similarities out.

\$\endgroup\$
  • \$\begingroup\$ thank you for your time! definitely will improve on these points and follow pep 8 guide \$\endgroup\$ – Bobby Mar 8 '17 at 1:24

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.