Suggestions on cleaning up Flask app

Question

I've written a little bit of code to pull data out of my FitBit and store it in a GCP database for further analysis. The project is available here, but what I'd like to ask about specifically is the Flask app & code I'm using to serve up a web interface for the project:

from datetime import date
import os

from flask import Flask, make_response, render_template, request
from flask_wtf import FlaskForm

from wtforms import StringField

from fitnick.activity.activity import Activity
from fitnick.database.database import Database
from fitnick.heart_rate.time_series import HeartRateTimeSeries
from fitnick.heart_rate.models import heart_daily_table

app = Flask(__name__)
SECRET_KEY = os.urandom(32)
app.config['SECRET_KEY'] = SECRET_KEY

month_options = [i for i in range(1, 13)]
day_options = [i for i in range(1, 32)]
year_options = range(2020, 2021)


class DateForm(FlaskForm):
    date = StringField('date')


@app.route("/", methods=['GET'])
def index():
    """
    Currently serves as the endpoint for the get_heart_rate_zone methods, even
    though it's currently set to the index page
    :return:
    """
    heart_rate_zone = HeartRateTimeSeries(config={'database': 'fitbit'})
    statement = heart_daily_table.select().where(heart_daily_table.columns.date == str(date.today()))
    rows = [i for i in Database(database='fitbit', schema='heart').engine.execute(statement)]
    #  retrieve rows for today already in database, if there are none then get rows via fitnick
    if len(rows) == 0:
        rows = heart_rate_zone.get_heart_rate_zone_for_day(database='fitbit')

    rows = [i for i in rows]

    form = DateForm(request.form)

    if request.method == 'GET':
        return render_template(
            template_name_or_list="index.html",
            rows=rows,
            form=form,
            month_options=month_options,
            day_options=day_options,
            year_options=year_options
        )


def set_search_date(request, search_date):
    if request.method == 'POST':
        # collect search date information from the dropdown forms if they're all supplied.
        if all([request.form.get('month_options'), request.form.get('day_options'), request.form.get('year_options')]):
            search_date = '-'.join(
                [f"{request.form['year_options']}",
                 f"{request.form['month_options']}".zfill(2),
                 f"{request.form['day_options']}".zfill(2)])
        else:
            # use the search_date value we set in lines 59-62
            pass

    return search_date


@app.route("/get_heart_rate_zone_today", methods=['GET', 'POST'])
def get_heart_rate_zone_today():
    """
    Endpoint for getting heart rate zone data from the FitBit API.
    :return:
    """
    heart_rate_zone = HeartRateTimeSeries(config={'database': 'fitbit'})
    form = DateForm(request.form)
    value = 'Updated heart rate zone data for {}.'

    if form.date._value():  # set search_date, default to today if none supplied
        search_date = form.date._value()
    else:
        search_date = str(date.today())

    if request.method == 'POST':
        # collect search date information from the dropdown forms if they're all supplied.
        search_date = set_search_date(request, search_date)
        rows = heart_rate_zone.get_heart_rate_zone_for_day(
            database='fitbit',
            target_date=search_date)
        rows = [i for i in rows]
    else:  # request.method == 'GET'
        # no date supplied, just return data for today.
        heart_rate_zone.config = {'base_date': date.today(), 'period': '1d'}
        statement = heart_daily_table.select().where(heart_daily_table.columns.date == str(date.today()))
        rows = Database(database='fitbit', schema='heart').engine.execute(statement)

    return render_template(
        template_name_or_list="index.html",
        value=value.format(search_date),
        rows=rows,
        form=form,
        month_options=month_options,
        day_options=day_options,
        year_options=year_options
    )


@app.route("/get_activity_today", methods=['GET', 'POST'])
def get_activity_today():
    """
    Endpoint for getting activity data for a given date from the FitBit API.
    :return:
    """

    activity = Activity(config={'database': 'fitbit'})
    form = DateForm(request.form)
    search_date = form.date._value()

    if request.method == 'POST':
        search_date = set_search_date(request, search_date)
        row = activity.get_calories_for_day(day=search_date)
        value = 'Updated activity data for {}.'.format(search_date)
    else:
        row, value = {}, ''

    return render_template(
        template_name_or_list='activity.html',
        form=form,
        row=row,
        value=value,
        month_options=month_options,
        day_options=day_options,
        year_options=year_options
    )


@app.route('/<page_name>')
def other_page(page_name):
    """
    Stand-in endpoint for any undefined URL.
    :param page_name:
    :return:
    """
    response = make_response(f'The page named {page_name} does not exist.', 404)
    return response


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

Specifically, I'm trying to cut down on some code repetition. I have two pages currently, one for grabbing heart rate zone data and another for collecting activity data. When it comes to retrieving the date entered into the form on either page & then querying the database and returning the proper results, it seems like there's a lot of code repetition.

It feels like there should be a way to collapse the code in the index, get_heart_rate_zone_today and get_activity_today functions where essentially what is being checked is if the request is GET or POST and if it's POST, query the data for the given day from the FitBit API, add it to the database & return the data (when it's a GET request, we just return the data in the database). This section of the get_heart_rate_zone_today function is an example of what I mean:

if form.date._value():  # set search_date, default to today if none supplied
    search_date = form.date._value()
else:
    search_date = str(date.today())

if request.method == 'POST':
    # collect search date information from the dropdown forms if they're all supplied.
    search_date = set_search_date(request, search_date)
    rows = heart_rate_zone.get_heart_rate_zone_for_day(
        database='fitbit',
        target_date=search_date)
    rows = [i for i in rows]
else:  # request.method == 'GET'
    # no date supplied, just return data for today.
    heart_rate_zone.config = {'base_date': date.today(), 'period': '1d'}
    statement = heart_daily_table.select().where(heart_daily_table.columns.date == str(date.today()))
    rows = Database(database='fitbit', schema='heart').engine.execute(statement)

However, there are still enough differences from a code perspective (the different actions the code should take depending on if it's activity or heart rate zone data being retrieved) that the easiest way I've found thus far is to just write separate functions for each retrieval method.

I'd really appreciate it if someone could check out the Flask code I have below and get your thoughts on it, what I'd like to do and if it's the right idea for solving the 'problem' I have currently, which is the semi-duplicated code. Thank you for the time!

Answer 1

My advice would be to restructure the project by splitting it into several files. In this file you have everything mixed up together, you are instantiating the Flask instance, you define the routes, and you have functions in between.

This file can only grow over time and can become overwhelming, making code maintenance more tedious and difficult in the long run.

To start Flask the idea is to have a small launcher file that looks like this:

from flask import Flask

app = Flask(__name__)
app.config.from_object('config.Config')

and a config file that would look like this:

from os import environ, path
from dotenv import load_dotenv

basedir = path.abspath(path.dirname(__file__))
load_dotenv(path.join(basedir, '.env'))


class Config:
    """Base config."""
    SECRET_KEY = environ.get('SECRET_KEY')
    SESSION_COOKIE_NAME = environ.get('SESSION_COOKIE_NAME')
    STATIC_FOLDER = 'static'
    TEMPLATES_FOLDER = 'templates'


class ProdConfig(Config):
    FLASK_ENV = 'production'
    DEBUG = False
    TESTING = False
    DATABASE_URI = environ.get('PROD_DATABASE_URI')


class DevConfig(Config):
    FLASK_ENV = 'development'
    DEBUG = True
    TESTING = True
    DATABASE_URI = environ.get('DEV_DATABASE_URI')

Source and recommended reading: Configuring Your Flask App. I use a similar scheme to be able to switch easily between test and production environments, without touching the code base. Instead, changes are made to a config file. Note that in your app you have hardcoded: app.run(debug=True)

I would also keep the routes in a distinct file. A simplistic example to import your routes:

from flask import Flask

def create_app():
    """Initialize the core application."""
    app = Flask(__name__, instance_relative_config=False)
    app.config.from_object('config.Config')

    with app.app_context():
        # Include our Routes
        from . import routes

        return app

Source: Demystifying Flask’s Application Factory

If you have some helper functions they can go to another file as well. No need to clutter the codebase.

In short, my suggestion is to spend more time reading up on Flask and take the time to build a sound boilerplate that you can reuse for future projects.

I am not familiar with your database scheme but it is obvious there is repetition across your code eg: get_heart_rate_zone_for_day(database='fitbit') or: Database(database='fitbit', schema='heart'). The DB name is mentioned multiple times. It should be set up once and for all at the start of your application. At the very least you could have defined a constant at the top of your code.

I suppose something like this at the top of your code should do:

FITBIT_DB = Database(database='fitbit', schema='heart')

Then use FITBIT_DB from now on to refer to the DB. Just this should make the code ligher.

At the top of your code you have this:

month_options = [i for i in range(1, 13)]
day_options = [i for i in range(1, 32)]
year_options = range(2020, 2021)

I presume you have 3 combo boxes in your Jinja template. To make the code future-proof and not having to review it every year, you should make the year range dynamic:

from datetime import datetime
current_year = datetime.now().year
year_options = range(current_year, current_year + 1)

But it doesn't seem that you are using WTF for validating the dates. Dates like 30 Feb should trigger an error, even if the code does not crash and ends up returning no data.

Not sure this code is correct:

def set_search_date(request, search_date):
    if request.method == 'POST':
        # collect search date information from the dropdown forms if they're all supplied.
        if all([request.form.get('month_options'), request.form.get('day_options'), request.form.get('year_options')]):

First of all, there is no need to make it a list.

Reminder: what the all function does: check if all items in a list are True. So I would check that this code does what you really think it does. If in doubt you can dump the contents of the POST request and see for yourself what happens.

Answer 2

A few general suggestions, based on the code

1.- Organization. It could be a hacky project, but you should avoid doing db queries on the views, it will lead to hard to maintain code, as you are noticing now. Try to have a separate file to handle all queries, and then use if you want a function to communicate between them.stants defined above, which may suit bett

2.- Keep as little logic in the views as possible. Create functions you can reuse, for example to retrieve data from db, or to populate the search data

3.- Constants. If the months, years, etc.. are not going to change consider using tuples for them. Also the range you are creating only has a year on it

With this, it should lead to places where you can see reusable code, specially db queries

Good luck!

Answer 3

List comprehensions

This pattern is seen throughout your code:

[i for i in range(1, 13)]

In the case above, there's no need to make a list comprehension; simply hold onto the range directly. In a case like this:

rows = [i for i in rows]

you're better off calling the list constructor, i.e.

rows = list(rows)

or better, if you're able, the tuple constructor for immutable variables:

rows = tuple(rows)

Redundant method check

This:

if request.method == 'GET':

is written on an index method that only accepts GET, so the if is redundant and can be dropped.

set_search_date

This is a curious method. First, it doesn't set anything - it returns a date. Unless I'm missing something, rename this to be get_search_date. You can also combine your predicates:

if (
    request.method == 'POST'
    and all(
        f'{k}_options' in request.form
        for k in (
            'month', 'day', 'year',
        )
    )
):
    return '-'.join # ...

return search_date