7
\$\begingroup\$

While the following code works, it looks like trash. Furthermore, there are even more templates that I need to add.

Before I continue I should like to know what I can do to fix or improve the following code. I am not interested in docstrings or anything like that, give it time and I will add them. I am looking for a better way to structure this mess. 

import math
import smtplib
from typing import Tuple
from email.mime.text import MIMEText
from email.mime.multipart import MIMEMultipart


class MessageUser():
    port = 587
    host = 'smtp.gmail.com'
    to_addr = '*****'
    from_addr = '*****'
    password = '*****'
    model_outputs = {
        'eggs': 0.0,
        'milk': 0.0
    }
    base_t_msg_nothing_to_buy = ('Dear James,\n\nYour fridge is looking quite'
                                 ' full!\nIt looks like you are all set for'
                                 ' the upcoming days.\n\nBest regards,\nAI')
    base_h_msg_nothing_to_buy = '''
    <html>
    <body>
    <p>
    Dear James,<br><br>
    Your fridge is looking quite full!.<br>
    It looks like you are all set for the upcoming days.<br><br>
    Best regards,<br>
    AI
    </p>
    </body>
    </html>
    '''
    base_t_msg_suggestions = ('Dear James,\n\nYour fridge is looking ok,'
                              ' however, I should advise you to buy the\n'
                              'following products as fast as possible:\n\n{}'
                              '\nBest regards,\nAI')
    base_h_msg_suggestions = '''
    <html>
    <body>
    <p>
    Dear James,<br><br>
    Your fridge is looking ok, however, I should advise you to buy the<br>
    following products as fast as possible:<br><br>{}<br>
    Best regards,<br>
    AI
    </p>
    </body>
    </html>
    '''
    base_t_msg_order_details = ()
    base_h_msg_order_details = ''''''

    def __init__(self, eggs_output, milk_output) -> None:
        self.model_outputs['eggs'] = eggs_output
        self.model_outputs['milk'] = milk_output

    def make_message(self) -> Tuple[str]:
        products_to_send = {}
        for attr, value in self.model_outputs.items():
            if math.floor(value) <= 2:
                products_to_send[attr] = value

        if len(products_to_send) <= 0:
            return (self.base_t_msg_nothing_to_buy,
                    self.base_h_msg_nothing_to_buy)
        elif len(products_to_send) <= 2:
            text_format = ''
            html_format = ''
            for attr, value in products_to_send.items():
                text_format += (f'\t{attr.title()}: {math.floor(value)} to'
                                f' {math.ceil(value)} days remaining.\n')
                html_format += (f'<strong>{attr.title()}</strong>:'
                                f' {math.floor(value)} to {math.ceil(value)}'
                                ' days remaining.<br>')
            return (self.base_t_msg_suggestions.format(text_format),
                    self.base_h_msg_suggestions.format(html_format))
        else:
            pass

    def send_email(self) -> bool:
        try:
            the_msg = MIMEMultipart('alternative')
            the_msg['Subject'] = 'Grocery List - AI'
            the_msg['From'] = self.from_addr
            the_msg['To'] = self.to_addr
            (text_msg, html_msg) = self.make_message()
            msg_part_1 = MIMEText(text_msg, 'plain')
            msg_part_2 = MIMEText(html_msg, 'html')
            the_msg.attach(msg_part_1)
            the_msg.attach(msg_part_2)
            email_conn = smtplib.SMTP(self.host, self.port)
            email_conn.ehlo()
            email_conn.starttls()
            email_conn.login(self.from_addr, self.password)
            email_conn.sendmail(
                self.from_addr, self.to_addr, the_msg.as_string())
            email_conn.close()
            return True
        except smtplib.SMTPException:
            print('Could not send email.')
            return False
Improve this question
\$\endgroup\$

1 Answer 1

Reset to default
10
\$\begingroup\$

The goals of this will be to make the code more readable and maintainable.

  1. Don't put templates and credentials in your code.

    Most of the class-level declarations of MessageUser are setting up stuff. You should store your configurable parameters in a configuration file. Check out the Python documentation for more info. This ought to be enough. Note it does allow storing multi-line strings. Definitely do not ever store credentials in your code.

  2. Use data structures for related data.

    Rather than having multiple variables, for each message, store the pair (text, html) of each message in a dictionary or a tuple. That way you can just pass in the data structure to the function that generates the message. This will also make it easier to add additional message types in the future.

  3. Document your code and name variables properly.

    You know about docstrings, great. But also you should make sure to use comments to explain the purpose of why you're doing things a certain way, and also make sure to choose meaningful names.

  4. Only use classes if the code benefits as a result

    MessageUser is kind of cryptic. It's not a class that represents a user, and it's not a class that represents a message. It's a class that...what actually is the purpose of the class? Can you describe it in a sentence? Probably it doesn't need to exist, and should just be a few functions. A class should represent one thing or concept. This seems to me to be a bunch of unrelated functions -- perhaps their responsibilities should be more clearly delineated. Writing comments and docstrings will help you figure out what the actual purpose of your code is.

  5. A few other lines that look strange

    for attr, value in self.model_outputs.items():

    Choose better variable names. What does model_outputs actually represent? How about for product, quantity ?

    if math.floor(value) <= 2:

    What is going on here? Where does 2 come from? This number should probably be read in from the config file. Why is there a math.floor? Why don't you just compare it with 3.0?

    if len(products_to_send) <= 0:

    The standard pythonic way to check if it's empty is to use "not": if not products_to_send:

    else:
    pass

    Why is this code here? It only reaches this point if len(products_to_send)>2. I don't understand this at all.

  6. Don't make try blocks larger than necessary

    Putting code in a try block implies that you expect it to fail and that you are prepared to handle the failure. But the only exception you're handling is if the SMTP connection goes wrong. So only the SMTP stuff should be in the try block

Improve this answer
\$\endgroup\$
3
  • \$\begingroup\$ Hey, thanks again for the great answer. Let me get a couple of things straight. Every config variable and "Magic Numbers" like the 2 should go in a config file? I guess that every config from every module shares the same file? The other thing is the templates, you told me to not put the templates in my code, where should I put them? In the same config file or a different email_template.py file? The way that the templates are structured, is there a better way to write them or is this ok? @Snowbody \$\endgroup\$
    – Vera Perrone
    Dec 12, 2017 at 10:05
  • 1
    \$\begingroup\$ No, not every variable and number, but ones that it makes sense to. Stuff that could potentially change. Stuff you don't want to explain in your code. I don't know what the purpose of the 2 is so I can't advise on it. As for your templating, what you're doing is perfect for this simple case; if you wanted something more powerful check this link Put the templates in a config file, same or different, whatever makes sense to you. Since it's just text substitution you don't need another .py file. \$\endgroup\$
    – Snowbody
    Dec 12, 2017 at 14:57
  • \$\begingroup\$ Thanks for the accept, but traditionally on codereview it's best to wait 24-48 hours before accepting an answer, just in case someone else wants to contribute. By accepting an answer early you're basically telling everyone you don't want any more advice. \$\endgroup\$
    – Snowbody
    Dec 12, 2017 at 16:19

Your Answer

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

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