4
\$\begingroup\$

I am making an app that shows some pictures and you can like each picture. I am sending a POST request from the Android device. In my backend I have a Python script that takes parameters category and URL of the picture, opens a JSON file, and modifies the "likes" value.

This is the Python script:

#!/usr/bin/python
# Import modules for CGI handling 
import cgi, cgitb, json

# Create instance of FieldStorage 
form = cgi.FieldStorage() 

# Get data from fields
category= form.getvalue('category')
url= form.getvalue('url')

#Gets the file path,depending on the category
filepath='/home2/public_html/app/%s/%s.json' % (category,category)

# Open a file
with open(filepath) as data_file:    
data = json.load(data_file)

# Finds the picture with the specific url and updates the likes value.
for picture in data:
    if picture ["url"] == url:
        likes = int(picture["likes"])
        picture["likes"]=str(likes+1)
        break

# Writes the json file with the new value.
with open(filepath, 'w') as data_file:
    data_file.write(json.dumps(data))

My JSON looks like this:

{"url": "http://pictures.com/app/Funny/picture.jpg", "category": "Funny", "likes": "0"}

What can go wrong with this script? Are there any potential drawbacks? I am new to backend programming, so I don't know what can go wrong here.

\$\endgroup\$
3
\$\begingroup\$

I'm not sure what this program is trying to do, but I can give a few suggestions for style and improvements.

  1. Firstly, unless you're not using the python command to run this script on the command line, you don't need the #!/usr/bin/python.
  2. Secondly, I'd separate the imports out on each separate line like so.

import cgi
import cgitb
import json

  1. This is in case an error occurs with the imports, if thats the case, the error will be easier to find.
  2. After that I'd separate certain things into functions and add docstrings. A few things I might want to separate into functions is opening the file, updating the like values, and writing to the JSON file. This gives your program a bit of re-usability.
  3. My final bit of advice would be to remove or improve un-needed comments such as the following.

# Create instance of FieldStorage
# Open a file
# Import modules for CGI handling

I'd also recommend checking out Python's style guide, PEP8. Hope this helps!

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks,for the comment.How about some kind of error checking,exceptions,is there really something that can go wrong,when calling this script via post request? \$\endgroup\$ – risto Apr 15 '15 at 15:47
3
\$\begingroup\$

Firstly, this isn't going to handle concurrent requests. You are reading and writing to a file without a lock. This means that if two requests read filepath they will see 1 likes and both write 2 likes. This is a classic concurrency problem and would be best solved with a proper database (even sqlite would do for something quick). This will allow you to make your integer increments atomic without the pain-in-the-rear filesystem issues.

Secondly, you will want to sanitise your input. Though json.load will probably crash when reading anything other than JSON you can never be too sure. You will want to escape anything malicious from the path such as /../ parts of the string or resolve the path to an absolute path and check the parent directory with the os.path API.

Thirdly, while this will get something working quickly, you should really look at using a lightweight framework such as Flask/Werkzeug or webpy. You will find that the tools and idioms in these frameworks will be helpful and educational.

| improve this answer | |
\$\endgroup\$

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.