2
\$\begingroup\$

Would love some feedback on how to improve my code. It might not be as readable as I'd like in places, but it currently works. Note that this code will be adapted to work on a different spreadsheet, so making this extensible would be amazing. Also, is there any reason to put this code into a class? I come from a scripting background, and OOP concepts are new to me.

This is the data that it looks at:

spreadsheet example

The code gets a row which it uses its index to turn FALSE to TRUE after it sends an email. The email isn't implemented yet, but will be soon. Any suggestions for if there's something other then smtplib on python would be appreciated, as I might want to just send the email without hooking up an account (like mail command on Linux).

import gspread
import pprint
from oauth2client.service_account import ServiceAccountCredentials


def setup_connection(spreadsheet):
    scope = ['https://spreadsheets.google.com/feeds']
    creds = ServiceAccountCredentials.from_json_keyfile_name('client_secret.json', scope)
    client = gspread.authorize(creds)
    worksheet = client.open(spreadsheet).sheet1
    return worksheet


def send_email(is_sent, is_passing):
    """
    Takes in two parameters to determine if
    1. the email is sent
    2. did the quiz-taker pass?

    Returns true if the email was sent and false if not. Will probably need to be updated, too!
    """
    if not is_sent:
        print("sending email...")
        if is_passing:
            print("you passed! congratulations :)")
        else:
            print("sorry, but you didn't pass yet! We'd really love to see you at the MakerSpace, so make sure to try again!")
            return True
    else:
        print("email already sent.")
        return False


def main():
    worksheet = setup_connection('CoMotion Quiz Test Automation')
    rows = worksheet.get_all_values()
    index = 0
    for row in rows:
        index = index + 1
        if (row[4] == "FALSE"):
            is_passing = (float(row[0])/30 > 0.8)
            is_sent = False
            if (send_email(is_sent, is_passing)):
                worksheet.update_acell('E' + str(index), "TRUE") # Updates the cell to reflect true

            else:
                print("these are not the droids you\'re looking for...")


main()
\$\endgroup\$
2
\$\begingroup\$

Few minor things to improve:

The code of the main() function with the above mentioned improvements applied:

for index, (score, *_, is_email_sent) in enumerate(rows):
    if is_email_sent != "FALSE":
        continue

    is_passing, is_sent = float(score) / 30 > 0.8, False
    if send_email(is_sent, is_passing):
        worksheet.update_acell(f'E{index}', "TRUE")  # Updates the cell to reflect true
    else:
        print("these are not the droids you\'re looking for...")

I am, also, not really sure about that is_sent variable and send_email() having this argument. I think send_email should simply be responsible for sending an email and nothing - do one thing (Single Responsibility Principle). Printing out a message if an email was already sent should be done outside of the function.

\$\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.