8
\$\begingroup\$

I've created a small Flash Briefing Alexa skill that reports the top 5 trending topics in Machine Learning from the My Bridge service.

The feed for the Flash Briefing skill can be pointed to an external endpoint, but the response has to be in a certain format.

Sample:

[    
    {
        mainText: "Posted by Hui Fang, Software Engineer, Machine Perception Machine learning (ML) excels in many areas with well defined goals. Tasks where ...",
        redirectionUrl: "https://research.googleblog.com/2017/07/using-deep-learning-to-create.html",
        titleText: "Using Deep Learning to Create Professional-Level Photographs",
        uid: "200561",
        updateDate: "2017-07-13T20:11:59.000000Z"
    },
    ...
]

To expose an endpoint I've created a small Flask app (deployed to Heroku, runs on gunicorn), that interacts with a My Bridge API via the requests module:

from datetime import datetime
import logging

from flask import Flask, jsonify
import requests


ML_ID_URL = "https://mybridge-backend.herokuapp.com/api/v1/skills/mapping/machine-learning"
ML_TOP_URL = "https://mybridge-backend.herokuapp.com/api/v1/knowledge/skills/{id}?offset=0&limit=5&sort="

logger = logging.getLogger()
logger.setLevel(logging.INFO)

app = Flask(__name__)


@app.route("/")
def main():
    with requests.Session() as session:
        session.headers = {"User-Agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36"}

        # get topic ID
        response = session.get(ML_ID_URL)
        data = response.json()

        ml_topic_id = data["body"]["id"]

        # get top 5 trending topics
        response = session.get(ML_TOP_URL.format(id=ml_topic_id))
        data = response.json()

        return jsonify([
            {
                "uid": str(topic["id"]),
                "updateDate": datetime.strptime(topic["reg_date"], "%Y-%m-%d %H:%M:%S.%f").strftime("%Y-%m-%dT%H:%M:%S.%fZ"),  # "2016-04-10T00:00:00.0Z"
                "titleText": topic["title"],
                "mainText": topic["excerpt"],
                "redirectionUrl": topic["externalURL"]
            }
            for topic in data['body']
        ])


if __name__ == '__main__':
    app.run(debug=True, use_reloader=True)

What would you improve code style and code organization wise?

\$\endgroup\$
8
+50
\$\begingroup\$

Structure

Nothing! The code seems fairly straightforward and clear, and I was able to understand what you’re doing even if I didn't study flask too much. So I don’t think there’s anything wrong with the structure.

Code style

One small nitpick: some of your lines are too long (>79 characters). From PEP8:

Limit all lines to a maximum of 79 characters.

Anyway, as a personal preference, I like to stick to PyCharm's recommendation and keep it < 120. But even with that in mind, you still have one line which doesn't fit.

Do this instead (for <120):

session.headers = {
    "User-Agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) "
                  "Chrome/59.0.3071.115 Safari/537.36"
}

Or:

session.headers = {
        "User-Agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5)" 
                      "AppleWebKit/537.36 (KHTML, like Gecko) "
                      "Chrome/59.0.3071.115 Safari/537.36"
    }

Anyway, that's really subjective and it didn't bother me that you kept the entire User-Agent on the same line.

The same observation is also applicable for updateDate.


Flask-RESTful API

Apparently, you're trying to create some sort of RESTful API. Luckily, flask let's you do this by installing their Flask-RESTful API which you can do by running pip install flask-restful.

Flask-RESTful is a powerful extension for Flask that was designed to help you build complex RESTful APIs. It follows similar design principles to Flask so it is really easy (IMO) to pick up. Now, you might wonder why you should use this: well, your code was quite straightforward to implement but as the API grows in complexity it is easy to see this code becoming very complicated, very quickly!

\$\endgroup\$
5
\$\begingroup\$

You could break your main function into multiple functions. Doing that will not even make main() much more clearer but also you will get some re-useable functions.

Having these tiny functions allow you to mock them easily in tests and also allows them to be tested them separately.

I have also added response.raise_for_status() calls to raise an HTTPError if the API returns 4xx or 5xx status code. In these situations it's rare that you would get the expected key-value pairs.

Lastly if your code is going to stay in memory then having module level session will help as well.

from datetime import datetime
import logging

from flask import Flask, jsonify
import requests


ML_ID_URL = "https://mybridge-backend.herokuapp.com/api/v1/skills/mapping/machine-learning"
ML_TOP_URL = "https://mybridge-backend.herokuapp.com/api/v1/knowledge/skills/{id}?offset=0&limit={limit}&sort="

logger = logging.getLogger()
logger.setLevel(logging.INFO)

app = Flask(__name__)
session = requests.Session()
session.headers = {"User-Agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36"}


def get_topic_id():
    response = session.get(ML_ID_URL)
    response.raise_for_status()
    data = response.json()
    return data["body"]["id"]


def get_trending_topics(ml_topic_id, limit=5):
    response = session.get(ML_TOP_URL.format(id=ml_topic_id, limit=limit))
    response.raise_for_status()
    return response.json()


@app.route("/")
def main():
    ml_topic_id = get_topic_id()
    trending_topics = get_trending_topics(ml_topic_id)

    return jsonify([
            {
                "uid": str(topic["id"]),
                "updateDate": datetime.strptime(topic["reg_date"], "%Y-%m-%d %H:%M:%S.%f").strftime("%Y-%m-%dT%H:%M:%S.%fZ"),
                "titleText": topic["title"],
                "mainText": topic["excerpt"],
                "redirectionUrl": topic["externalURL"]
            }
            for topic in trending_topics['body']
        ])


if __name__ == '__main__':
    app.run(debug=True, use_reloader=True)
\$\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.