4
\$\begingroup\$

For the last couple of months I've been working on a python script that pulls data from a specific Trello list and sums up the numeric values by list (lists are split up into months). I've worked on making my code pythonic, basic functionality, separating functionality into distinct functions. It currently only works with a specific list and board. Things I plan to work on is; better error handling with network connections and introducing unit testing. Two things I have little experience with.

EDIT: the keys.txt file is formatted as follows

token=token

api_key=key

#!/usr/bin/python
from trollop import TrelloConnection

import logging
from math import ceil
from time import strftime

import seaborn as sns
import matplotlib.pyplot as plt

# Lots of issues with Python3. Lots of unicode, string errors, Just switched to
# py2. should try to use dicts for {name: cost} and to  practice using dicts

# TODO: Data Visualization, Error/exception handling
# TODO: clean up code, get feedback on reddit, maybe use args for spec params
# TODO: set up cron job to run every month
k = list()
with open('keys.txt', 'r') as keys:
    for line in keys:
        tmp = line.split('=')[1]
        tmp = tmp.rstrip()
        k.append(tmp)
token = k[0]
api_key = k[1]


# idBoard': '577b17583e5d17ee55b20e44',
# idList': '577b17583e5d17ee55b20e45',
# Set up basic logging
logging.basicConfig(format='%(levelname)s %(message)s',
                    level=logging.INFO, filename='DEBUG.log',
                    filemode='w')
# Establish connection
conn = TrelloConnection(api_key, token)
MONTHS = ['Jan', 'Feb', 'March', 'April', 'May', 'June', 'July', 'Aug',
          'Sept', 'Oct', 'Nov', 'Dec']


logging.info("Iterating through boards...")


def get_total_per_month(month, board_list):
    costs = 0.0
    month = month.lower()
    for lst in board_list:
        if month in lst.name.lower():
            for crd in lst.cards:
                costs += float(crd.name.split('-')[1])
    return ceil(costs)


def first_of_the_month():
    day = strftime("%d")
    if '1' is day:
        # TODO: check to see if it's a new month and add a list
        pass



def get_yearly_average(totals):
    sum = 0.0
    count = 0
    for month in totals:
        if month != 0.0:
            count = count + 1
            sum += month
            # print month
    year_average = sum / count
    print 'year ave ' + str(year_average)
    return year_average


def plot(totals, average):
    sns.set(style='white', font_scale=1.5)
    plt.title('Monthly Food Expenses')
    plt.xlabel('Months')
    plt.ylabel('Pesos')
    sns.barplot(x=MONTHS, y=totals)
    plt.show()


def main():
    costs = list()
    names = list()

    board = conn.get_board('BE89pW61')
    totals = [get_total_per_month(month, board.lists) for month in MONTHS]
    print totals
    average = get_yearly_average(totals)
    logging.info(totals)
    logging.debug('Board list: {}'.format(board.lists))
    plot(totals, average)
if __name__ == '__main__':
    main()
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Remove code from the global scope. You should try to keep global scope to just functions and classes. This is as adding data here means any function can change it, and so you can no-longer trust it's state.

The only time I use globals that contain data, is when I use a constant. Take MONTHS. Which is a good global variable.

I'd add your settings as a global constant too. You should probably use a dictionary, as then you don't have to use dodgy indexing. But, using a dictionary means that you'd have to use SETTINGS['token'], rather than SETTINGS.token. And so I'd make a new class that has this sugar:

class FrozenDict(object):
    def __init__(self, *args, **kwargs):
        # Hack to bypass `__setattr__`.
        self.__dict__['d'] = dict(*args, **kwargs)

    def __getattr__(self, key):
        return self.d[key]

    def __setattr__(self, key, val):
        raise TypeError('FrozenDict does not support setting attributes.')

    def __delattr__(self, key, val):
        raise TypeError('FrozenDict does not support deleting attributes.')

After this you may want to put getting settings into it's own function. As you wrote:

TODO: clean up code, get feedback on reddit, maybe use args for spec params

This new function should allow you to use both a file and take arguments from the command line. I won't go into how to do this, as Reddit probably did. But I'd use argparse. But back to your code. I'd change the construction of the list to be entirely in the with. As with statements aren't in a different scope. And I would use a list comprehension to build the list, rather than using append. This can get you:

def read_settings():
    with open('keys.txt', 'r') as keys:
        k = [line.split('=')[1].rstrip() for line in keys]
        token = k[0]
        api_key = k[1]
    return FrozenDict({
        'token': k[0],
        'api_key': k[1],
        'board': 'BE89pW61'
    })

Nice and simple. I don't know what keys.txt is structured like, but I would recommend that you instead format it so it's:

token=...
api_key=...
board=BE89pW61

Which would allow you to instead use a dictionary comprehension. And would allow you to not be so ridged on how the settings are provided. Using the second argument to str.split. And taking advantage of dict you could get:

def read_settings():
    with open('keys.txt', 'r') as keys:
        d = dict(line.split('=', 1) for line in keys)
    return FrozenDict(d)

After this I'd clean up both get_total_per_month and get_yearly_average. Instead of performing the addition yourself you can instead use sum. And to count the amount of items in an array you can use len. This allows you to change get_yearly_average to a list comprehension, and then just a sum divided by its length. And allows you to change get_total_per_month to a comprehension, passed to sum. I'd also remove the ceil in get_total_per_month as you should change the data when outputting not in it's internal state. And leads to \$\pm12\$ accuracy, as if every month is \$0.01\$ you'll say they're \$1\$, and \$12 \dot{} 0.01 = 0.12\$, but \$12 \dot{} 1 = 12\$. Which is absurdly wrong.

Finally I'd remove your prints as you're using logging. It makes no sense to use print and Pythons logging library to log the programs state. You should only use print as a means to interact with the user, such as asking if they want to use keys.txt as their settings file.

This can leave you with:

#!/usr/bin/python

# Lots of issues with Python3. Lots of unicode, string errors, Just switched to
# py2. should try to use dicts for {name: cost} and to  practice using dicts

# TODO: Data Visualization, Error/exception handling
# TODO: clean up code, get feedback on reddit, maybe use args for spec params
# TODO: set up cron job to run every month

import logging

from trollop import TrelloConnection
import seaborn as sns
import matplotlib.pyplot as plt

MONTHS = ['Jan', 'Feb', 'March', 'April', 'May', 'June', 'July', 'Aug',
          'Sept', 'Oct', 'Nov', 'Dec']


class FrozenDict(object):
    def __init__(self, *args, **kwargs):
        self.__dict__['d'] = dict(*args, **kwargs)

    def __getattr__(self, key):
        return self.d[key]

    def __setattr__(self, key, val):
        raise TypeError('FrozenDict does not support setting attributes.')

    def __delattr__(self, key, val):
        raise TypeError('FrozenDict does not support deleting attributes.')


def read_settings():
    with open('keys.txt', 'r') as keys:
        k = [line.split('=')[1].rstrip() for line in keys]
        token = k[0]
        api_key = k[1]
    return FrozenDict({
        'token': k[0],
        'api_key': k[1],
        'board': 'BE89pW61'
    })


def get_total_per_month(month, board_list):
    month = month.lower()
    return sum(
        float(crd.name.split('-')[1])
        for lst in board_list
        if month in lst.name.lower()
        for crd in lst.cards
    )


def get_yearly_average(totals):
    totals = [t for t in totals if t != 0]
    return sum(totals) / len(totals)


def plot(totals, average):
    sns.set(style='white', font_scale=1.5)
    plt.title('Monthly Food Expenses')
    plt.xlabel('Months')
    plt.ylabel('Pesos')
    sns.barplot(x=MONTHS, y=totals)
    plt.show()


def main():
    # Establish connection
    conn = TrelloConnection(SETTINGS.api_key, SETTINGS.token)
    board = conn.get_board(SETTINGS.board)
    totals = [get_total_per_month(month, board.lists) for month in MONTHS]
    average = get_yearly_average(totals)
    logging.info(totals)
    logging.debug('Board list: {}'.format(board.lists))
    plot(totals, average)


if __name__ == '__main__':
    SETTINGS = read_settings()

    # Set up basic logging
    logging.basicConfig(format='%(levelname)s %(message)s',
                        level=logging.INFO, filename='DEBUG.log',
                        filemode='w')

    logging.info("Iterating through boards...")
    main()
\$\endgroup\$
5
  • \$\begingroup\$ Why not use tuple-assignment and do token, api_key = [line.split('=')[1].rstrip() for line in keys]? \$\endgroup\$
    – Graipher
    Commented Nov 30, 2016 at 10:31
  • \$\begingroup\$ @Graipher Because you can't use a, b, *c = [...] in Python2, and we don't know if there are more than two lines. I erred on the side of caution \$\endgroup\$
    – Peilonrayz
    Commented Nov 30, 2016 at 10:32
  • \$\begingroup\$ OK, fair enough. \$\endgroup\$
    – Graipher
    Commented Nov 30, 2016 at 10:33
  • \$\begingroup\$ @Peilonrayz Is their a better way of storing and retrieving my credentials? Before I just had them hardcoded in the code. \$\endgroup\$ Commented Dec 1, 2016 at 19:18
  • \$\begingroup\$ You could put them in the settings file, or if you don't like that idea, you can put them at the top of your file as constants. \$\endgroup\$
    – Peilonrayz
    Commented Dec 1, 2016 at 19:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.