5
\$\begingroup\$

This script sends and receives text messages and logs responses in a google sheet. Demo Gif here: https://github.com/RyGuy96/HealthTracker.

I'm a self-taught beginner coder just starting my formal education in college. I've never had an experienced programmer look over my work; it was suggested to me that I needed to start building my professional profile. I'd be very grateful if someone would look through this script and:

  1. Tell me if there are any glaring syntactical errors or best practices I've ignored.
  2. Suggest any obvious ways it might be improved in terms of readability or speed.

Thank you, I really do appreciate any and all advice! This is my first post here - if there something I've missed in the guidelines please let me know.

#!/usr/bin/env python

"""receiver.py: Receives an sms message to a specifind number; parses and saves data in a Google Spreadsheet."""

from flask import Flask, request
from twilio.twiml.messaging_response import MessagingResponse
import gspread
import re
import datetime
import time
from twilio.rest import Client
from oauth2client.service_account import ServiceAccountCredentials
import pytz


def sms_sender(message_text: str, recipient_num: str) -> None:
    """Define credentials for Twilio API sms."""

    # Find these values at https://twilio.com/user/account (note that this is not a secure method)
    account_sid = "YOUR_SID"
    auth_token = "YOUR_AUTH_TOKEN"
    client = Client(account_sid, auth_token)
    message = client.api.account.messages.create(to= recipient_num,
                                                 from_="YOUR_TWILIO_NUMBER",
                                                 body= message_text)


def open_spreadsheet() -> object:
    """Authorize Google Drive access and return spreadsheet reference."""

    scope = ['https://spreadsheets.google.com/feeds', 'https://www.googleapis.com/auth/drive']
    credentials = ServiceAccountCredentials.from_json_keyfile_name('JSON_FILE', scope)
    gc = gspread.authorize(credentials)
    wks_health = gc.open("Health Tracker").sheet1
    return wks_health


def get_date() -> datetime:
    utc_now = pytz.utc.localize(datetime.datetime.utcnow())
    return datetime.datetime.strftime(utc_now.astimezone(pytz.timezone("America/Chicago")), '%Y-%m-%d')


def parse_sms(sms_reply: str) -> dict:
    """Return single dictionary of relevant values in sms."""

    # ratings
    ratings = re.compile(r"\b\d+\b")
    rating_vals = (ratings.findall(sms_reply)[:5])

    # added meds
    add = re.compile(r"\+(.*?\))")
    add_meds = add.findall(sms_reply)

    # removed meds
    remove = re.compile(r"\-(.*?\))")
    remove_meds = remove.findall(sms_reply)

    # notes
    note = re.compile(r"Note\((.*?)\)", flags=re.IGNORECASE)
    add_note = [] if not note.findall(sms_reply) else note.findall(sms_reply)[0]

    # help
    helpme_variants = ["help me", "helpme", "help-me"]
    display_help = any([re.findall(i, sms_reply, flags=re.IGNORECASE) for i in helpme_variants])

    # see meds
    med_variants = ["seeMeds", "see-meds", "see meds"]
    display_meds = any([re.findall(i, sms_reply, flags=re.IGNORECASE) for i in med_variants])

    parsed_response = {"ratings": rating_vals, "add meds": add_meds, "remove meds": remove_meds, "notes": add_note,
                       "display help": display_help, "display meds": display_meds}

    return parsed_response


def get_current_meds() -> str:
    wks_health = open_spreadsheet()
    time.sleep(4)
    return wks_health.acell('G2').value.strip('][').split(', ')


def validate_sms(parsed_response: dict) -> str:
    """Check sms and return 'Valid' or one or more error messages to be sent to user."""

    invalid_responses = []

    try:
        assert len(parsed_response["ratings"]) == 5
    except AssertionError:
        invalid_responses.append("Invalid number of ratings (there should be five)")

    current_meds = get_current_meds()
    try:
        for med in parsed_response["remove meds"]:
            assert med in current_meds
    except AssertionError:
        invalid_responses.append("Med to remove not found, see your meds by replying 'see-meds'")
    try:
        for med in parsed_response["add meds"]:
            assert med not in current_meds
    except AssertionError:
        invalid_responses.append("Med to be added already listed, see your meds by replying 'see-meds'")

    finally:
        if invalid_responses:
            return ", ".join(invalid_responses)
        else:
            return "Valid"


def record_sms(parsed_response: dict) -> None:
    """Log sms responses in Google Sheets."""

    note = parsed_response["notes"]
    remove = parsed_response["remove meds"]
    add = parsed_response["add meds"]
    current_meds = get_current_meds()
    revised_med_list = [med for med in current_meds if med not in remove] + add
    revised_med_list_formated = str(revised_med_list).replace("\'", "")

    line = [get_date()] + parsed_response["ratings"] + [revised_med_list_formated]
    line.append(note if note else "")

    wks_health = open_spreadsheet()
    wks_health.insert_row(line, value_input_option='USER_ENTERED', index=2)


def help_message() -> str:
    # change symptoms as you see fit - some refactoring will be required if you you want to track more or less than five
    message = "Respond to messages with: " \
              "\n1. Hours slept " \
              "\n2. Stress level (1-9) " \
              "\n3. Joints (1-9) " \
              "\n4. Energy (1-9) " \
              "\n5. Mood (1-9) " \
              "\n6. Add a note with NOTE(YOUR NOTE)* " \
              "\n7. Add a med with +MEDNAME(DOSE)* " \
              "\n8. Remove a med with -MEDNAME(DOSE)* " \
              "\n9. See all meds with 'see-meds'* " \
              "\n10. See this menu with 'help-me'*" \
              "\n*Optional values in response"
    return message


def see_meds_message() -> str:
    message = "Your current meds are: " + str(get_current_meds())
    return message


app = Flask(__name__)


@app.route("/sms", methods=['GET', 'POST'])
def main() -> str:
    """Listen for incoming sms and log content or reply with one or more error messages."""

    from_body = request.values.get('Body', None)

    resp = []
    while from_body is not None:

        try:
            sms = parse_sms(from_body)
        except:
            resp.append("issue parsing")
            break
        try:
            if sms['display help'] == True:
                resp.append(help_message())
                break
        except:
            resp.append("issue with help message")
            break
        try:
            if sms['display meds'] == True:
                resp.append(see_meds_message())
                break
        except:
            resp.append("issue with showing meds")
            break
        try:
            validation_val = validate_sms(sms)
        except:
            resp.append("issue validating")
            break
        try:
            if validation_val == "Valid":
                record_sms(sms)
                resp.append("Response recorded!")
                break
        except:
            resp.append("issue logging valid sms")
            break
        try:
            if validation_val != "Valid":
                resp.append(validation_val)
                break
        except:
            resp.append("issue logging invalid sms")
            break

    mess= MessagingResponse()
    mess.message(str(", ".join(resp)))

    return str(mess)


if __name__ == "__main__":
    app.run(debug=True)
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Regex compilation

Your code here:

# ratings
ratings = re.compile(r"\b\d+\b")
rating_vals = (ratings.findall(sms_reply)[:5])

# added meds
add = re.compile(r"\+(.*?\))")
add_meds = add.findall(sms_reply)

# removed meds
remove = re.compile(r"\-(.*?\))")
remove_meds = remove.findall(sms_reply)

gets only halfway to a good idea. It is useful to separate regex compilation from regex execution, but only if the compiled regexes are persisted. You can put them as constants in global scope. Then the compilation cost is only paid once.

Help me

helpme_variants = ["help me", "helpme", "help-me"]
display_help = any([re.findall(i, sms_reply, flags=re.IGNORECASE) for i in helpme_variants])

First of all: any doesn't require a list; you should be passing the generator to any directly.

Beyond that: you don't need "variants" or a generator at all; you can do this with another regex:

display_help_re = re.compile(r'help[ -]?me', flags=re.IGNORECASE)

# ...

display_help = display_help_re.search(sms_reply) is not None

The same is true of your med_variants.

Waiting for...?

time.sleep(4)

Why is this here? It's spooky. At the very least, drop a comment. More likely is that you should attempt some manner of polling if possible to detect when the given condition is met.

Magic values

'G2' is a magic value. At the least, it should be put into a named constant. More likely is that you should be making a named range in your spreadsheet and using that range, if this is at all possible.

5 is also a magic value here:

    assert len(parsed_response["ratings"]) == 5

Logic by exception

This:

try:
    for med in parsed_response["remove meds"]:
        assert med in current_meds
except AssertionError:
    invalid_responses.append("Med to remove not found, see your meds by replying 'see-meds'")

is awkward and not necessary. Just do the check yourself, and use a set instead:

parsed_meds = set(parsed_response['remove_meds'])
for missing_med in parsed_meds - current_meds:
   invalid_responses.append(f'Med to remove "{missing_med} not found; see your meds by replying "see-meds"')

Character escapes

"\'"

does not need an escape backslash.

String continuation

Rather than using backslashes here:

message = "Respond to messages with: " \
          "\n1. Hours slept " \
          "\n2. Stress level (1-9) " \
          "\n3. Joints (1-9) " \
          "\n4. Energy (1-9) " \
          "\n5. Mood (1-9) " \
          "\n6. Add a note with NOTE(YOUR NOTE)* " \
          "\n7. Add a med with +MEDNAME(DOSE)* " \
          "\n8. Remove a med with -MEDNAME(DOSE)* " \
          "\n9. See all meds with 'see-meds'* " \
          "\n10. See this menu with 'help-me'*" \
          "\n*Optional values in response"

Put the whole thing in parens and drop the backslashes.

f-strings

def see_meds_message() -> str:
    message = "Your current meds are: " + str(get_current_meds())
    return message

can be

def see_meds_message() -> str:
    return f'Your current meds are: {get_current_meds()}'

Implicit None

Drop the , None from this:

from_body = request.values.get('Body', None)

Because that's already the default for get.

Boolean comparison

        if sms['display help'] == True:

should simply be

        if sms['display help']:
\$\endgroup\$
2
  • 1
    \$\begingroup\$ You are a wizard, thank you so much Reinderien. The "magic" values 5 and G2 are used in multiple functions. I assume you would declare a function that returns those Constants? e.g. def NUM_OF_RATINGS(): return 5 and def MEDS_CELL(): return chr(ord('a') + 1 + NUM_OF_RATINGS()) + "2"? Would you group both the constant functions as methods in a constant class? \$\endgroup\$
    – RyGuy
    Oct 15, 2019 at 17:52
  • 1
    \$\begingroup\$ Nah; it's easier just to declare a globally-scoped constant variable, NUM_OF_RATINGS = 5. \$\endgroup\$
    – Reinderien
    Oct 15, 2019 at 19:54

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.