2
\$\begingroup\$

I am making an object-oriented python project for a smart mirror running on a raspberry pi. The code receives input from API's, formats the data and displays it on the mirror. I would like to know what I can improve on. Is my code up to industry standards?

Code:

from Tkinter import *
import locale
import threading
import time
import requests
import feedparser
import json
import traceback
import urllib2
import praw

from PIL import Image, ImageTk
from contextlib import contextmanager

#Font Variables
font_type = 'Helvetica'
font_colour = "White"
xlarge_text_size = 48
large_text_size = 30
medium_text_size = 20
small_text_size = 12
xsmall_text_size =8
#News Variables
NEWS_COUNTRY_CODE = 'au'
#Weather Variables
READ_API_KEY = 'D71A7607GOWJSZ6D'
CHANNEL_ID = 502804
#Reddit Variables
SUBREDDIT_SELECTION = 'technology'


class Clock(Frame):
    def __init__(self, parent, *args, **kwargs):
        Frame.__init__(self, parent, bg='black')
        #Time Label
        self.time1 = ''
        self.timeLbl = Label(self, font=(font_type, xlarge_text_size), fg=font_colour, bg="black")
        self.timeLbl.pack(side=TOP, anchor=E)
        #Day Of the Week label
        self.weekday1 = ''
        self.weekdayLbl = Label(self, font=(font_type,medium_text_size), fg=font_colour, bg="black")
        self.weekdayLbl.pack(side=TOP,anchor=E)
        #Date Label
        self.date1 = ''
        self.dateLbl = Label(self,font=(font_type,medium_text_size), fg=font_colour, bg="black")
        self.dateLbl.pack(side=TOP,anchor=E)
        self.tick()

    def tick(self):
        #Set Clock
        time2 = time.strftime('%H:%M')
        if time2 != self.time1:
            time1 = time2
            self.timeLbl.config(text=time2)
            self.timeLbl.after(200, self.tick)
        # Set Day of the Week
        weekday2 = time.strftime('%A')
        if weekday2 != self.weekday1:
            self.weekday1 = weekday2
            self.weekdayLbl.config(text=weekday2)
        # Set date
        date2 = time.strftime("%d %b, %Y")
        if date2 != self.date1:
            self.date1 = date2
            self.dateLbl.config(text=date2)
class Weather(Frame):
    def __init__(self, parent, *args, **kwargs):
        Frame.__init__(self, parent, bg='black')
        self.temperature = ''
        self.humidity = ''
        self.uv = ''
        self.apparenttemp = ''
        self.icon = ''

        self.degreeFrm = Frame(self, bg="black")
        self.degreeFrm.pack(side=TOP, anchor=W)

        self.temperatureLbl = Label(self.degreeFrm, font=('Helvetica', xlarge_text_size), fg="white", bg="black")
        self.temperatureLbl.pack(side=LEFT, anchor=N)

        self.uvLbl = Label(self, font=('Helvetica', large_text_size), fg="white", bg="black")
        self.uvLbl.pack(side=TOP, anchor=W)

        self.humidityLbl = Label(self, font=('Helvetica', medium_text_size),fg="white",bg="black")
        self.humidityLbl.pack(side=TOP, anchor=W)

        self.apparenttempLbl = Label(self, font=('Helvetica', medium_text_size), fg="white", bg="black")
        self.apparenttempLbl.pack(side=TOP, anchor=W)

        self.get_local_weather()

    def get_local_weather(self):
        try:
            degree_sign = u'\N{DEGREE SIGN}'
            tempval = ''
            humidval = ''
            uvval = ''
            apptempval = ''

            conn = urllib2.urlopen("http://api.thingspeak.com/channels/%s/feeds/last.json?api_key=%s" \
                           % (CHANNEL_ID, READ_API_KEY))

            response = conn.read()
            data = json.loads(response)
            conn.close()

            tempval = "%.2f%s" % (float(str(data['field1'])), degree_sign)
            humidval = "%s%.2f%s" % ("Humidity ", float(str(data['field2'])), "%")
            uvval = "%s%s" % ("UV Level ", int(data['field3']))
            apptempval = "%s%.2f%s" % ("Feel's like ", float(str(data['field4'])), degree_sign)

            if self.temperature != None:
                self.temperature = tempval
                self.temperatureLbl.config(text=tempval)

            if self.humidity != None:
                self.humidity = humidval
                self.humidityLbl.config(text=humidval)

            if self.uv != None:
                self.uv = uvval
                self.uvLbl.config(text=uvval)

            if self.apparenttemp != None:
                self.apparenttemp = apptempval
                self.apparenttempLbl.config(text=apptempval)

        except Exception as e:
            traceback.print_exc()
            print "Error: %s. Cannot get weather." % e

        self.after(500, self.get_local_weather)
class News(Frame):
    def __init__(self, parent, *args, **kwargs):
        Frame.__init__(self, parent, *args, **kwargs)
        self.config(bg='black')
        self.title = 'News'
        self.newsLbl = Label(self, text=self.title, font=('Helvetica', medium_text_size), fg="white", bg="black")
        self.newsLbl.pack(side=TOP, anchor=W)
        self.headlinesContainer = Frame(self, bg="black")
        self.headlinesContainer.pack(side=TOP)
        self.get_headlines()

    def get_headlines(self):
        try:
            for widget in self.headlinesContainer.winfo_children():
                widget.destroy()
            if NEWS_COUNTRY_CODE == None:
                headlines_url = "https://news.google.com/news?ned=au&output=rss"
            else:
                headlines_url = "https://news.google.com/news?ned=%s&output=rss" % NEWS_COUNTRY_CODE

            feed = feedparser.parse(headlines_url)

            for post in feed.entries[0:5]:
                headline = NewsHeadline(self.headlinesContainer, post.title)
                headline.pack(side=TOP, anchor=W)
        except Exception as e:
            traceback.print_exc()
            print "Error: %s. Cannot get news." % e

        self.after(600000, self.get_headlines)
class NewsHeadline(Frame):
    def __init__(self, parent, event_name=""):
        Frame.__init__(self, parent, bg='black')

        image = Image.open("assets/Newspaper.png")
        image = image.resize((25, 25), Image.ANTIALIAS)
        image = image.convert('RGB')
        photo = ImageTk.PhotoImage(image)

        self.iconLbl = Label(self, bg='black', image=photo)
        self.iconLbl.image = photo
        self.iconLbl.pack(side=LEFT, anchor=N)

        self.eventName = event_name
        self.eventNameLbl = Label(self, text=self.eventName, font=('Helvetica', small_text_size), fg="white", bg="black")
        self.eventNameLbl.pack(side=LEFT, anchor=N)
class Reddit(Frame):
    def __init__(self, parent, *args, **kwargs):
        Frame.__init__(self, parent, *args, **kwargs)
        # Reddit Title Label
        self.title = 'Reddit Top 1:'
        self.redditLbl = Label(self, text=self.title, font=(font_type, medium_text_size), fg=font_colour, bg="black")
        self.redditLbl.pack(side=TOP, anchor=W)
        # Reddit article label
        self.postContainer= Frame(self, bg="black")
        self.postContainer.pack(side=TOP)
        self.get_reddit_post()

    def get_reddit_post(self):
        try:
            reddit = praw.Reddit(client_id='someinfo',
                     client_secret='someinfo', password='someinfo',
                     user_agent='someinfo', username='someinfor')

            subreddit = reddit.subreddit(SUBREDDIT_SELECTION)
            top_subreddit = subreddit.hot(limit=3)

            for submission in top_subreddit:
                if not submission.stickied:
                    top_post = Reddit(self.postContainer,"%s" % (submission.title))
                    top_post.pack(side=TOP, anchor =W)

        except Exception as f:
            traceback.print_exc()
            print "Error: %s. This is a BIG REDDIT ERROR." % f
class FullscreenWindow:

    def __init__(self):
        self.tk = Tk()
        self.tk.configure(background='black')
        self.topFrame = Frame(self.tk, background = 'black')
        self.bottomFrame = Frame(self.tk, background = 'black')
        self.topFrame.pack(side = TOP, fill=BOTH, expand = YES)
        self.bottomFrame.pack(side = BOTTOM, fill=BOTH, expand = YES)
        self.state = False
        self.tk.bind("<Return>", self.toggle_fullscreen)
        self.tk.bind("<Escape>", self.end_fullscreen)
        # clock
        self.clock = Clock(self.topFrame)
        self.clock.pack(side=RIGHT, anchor=N, padx=100, pady=60)
        # weather
        self.weather = Weather(self.topFrame)
        self.weather.pack(side=LEFT, anchor=N, padx=100, pady=60)
        # news
        self.news = News(self.bottomFrame)
        self.news.pack(side=LEFT, anchor=S, padx=100, pady=60)
        # reddit
        self.reddit = Reddit(self.bottomFrame)
        self.reddit.pack(side = RIGHT, anchor=S, padx=100, pady=60)

    def toggle_fullscreen(self, event=None):
        self.state = not self.state  # Just toggling the boolean
        self.tk.attributes("-fullscreen", self.state)
        return "break"

    def end_fullscreen(self, event=None):
        self.state = False
        self.tk.attributes("-fullscreen", False)
        return "break"

if __name__ == '__main__':
    w = FullscreenWindow()
    w.tk.mainloop()
\$\endgroup\$
4
\$\begingroup\$

Remove unused imports

You import threading, requests, contextmanager, and locale, but never use them.

Don't use global imports.

Use import Tkinter as tk and then prefix tk classes and functions with tk. (eg: tk.Label(...), etc). PEP8 specifically recommends against wildcard imports. Even though many tkinter tutorials do it, the valid reasons spelled out by PEP8 still apply.

Use tkinter's font objects

If you're going to use custom fonts, create font objects and use them. The benefit of doing so is that it becomes trivial to change the fonts later (either later in coding time, or later in runtime).

For example:

from tkFont import Font
FONT = {
    'xlarge': Font(family="Helvetica", size=48),
    'large':  Font(family="Helvetica", sixe=30),
    'medium': Font(family="Helvetica", sixe=20),
    'small':  Font(family="Helvetica", sixe=12),
    'xsmall': Font(family="Helvetica", sixe=8),
}
...
self.timeLbl = Label(..., font=FONT['xlarge'], ...)
self.weekdayLbl = Label(..., font=FONT['medium'], ...)
...

If you want the user to be able to make the font bigger or smaller at runtime, it's trivial to do so because you only have to modify the font rather than modify every widget that uses the font.

Use 'after' wisely

Your function tick is called very 200ms, but what it displays changes only once a minute. I can understand wanting it to be fairly accurate, but if you're off by a few seconds does it really matter? At the very least, have it run once a second. That still will use considerably less CPU time than calling it 5 times a second.

Likewise, is it really necessary to update the weather data twice a second? Why not once every minute or every 5 minutes? Weather doesn't typically fluctuate much in such a short period of time.

Use more whitespace.

PEP8 gives good guidelines. For example, add two spaces between each class.

Separate widget creation from widget layout

In my experience, GUI code is much easier to maintain over time when layout code is grouped together.

For example, instead of this:

self.degreeFrm = Frame(self, bg="black")
self.degreeFrm.pack(side=TOP, anchor=W)

self.temperatureLbl = Label(self.degreeFrm, font=('Helvetica', xlarge_text_size), fg="white", bg="black")
self.temperatureLbl.pack(side=LEFT, anchor=N)

self.uvLbl = Label(self, font=('Helvetica', large_text_size), fg="white", bg="black")
self.uvLbl.pack(side=TOP, anchor=W)

self.humidityLbl = Label(self, font=('Helvetica', medium_text_size),fg="white",bg="black")
self.humidityLbl.pack(side=TOP, anchor=W)

self.apparenttempLbl = Label(self, font=('Helvetica', medium_text_size), fg="white", bg="black")
self.apparenttempLbl.pack(side=TOP, anchor=W)

... I recommend doing it like this:

self.degreeFrm = Frame(self, bg="black")
self.temperatureLbl = Label(self.degreeFrm, font=('Helvetica', xlarge_text_size), fg="white", bg="black")
self.temperatureLbl.pack(side=LEFT, anchor=N)

self.uvLbl = Label(self, font=('Helvetica', large_text_size), fg="white", bg="black")
self.humidityLbl = Label(self, font=('Helvetica', medium_text_size),fg="white",bg="black")
self.apparenttempLbl = Label(self, font=('Helvetica', medium_text_size), fg="white", bg="black")

self.degreeFrm.pack(side=TOP, anchor=W)
self.uvLbl.pack(side=TOP, anchor=W)
self.humidityLbl.pack(side=TOP, anchor=W)
self.apparenttempLbl.pack(side=TOP, anchor=W)

In my opinion, this makes it much easier to see which widgets are grouped together in self and which are not. Plus, layout code is often interdependent -- if you change the way you layout one widget, you may have to change others in the same parent. Having them grouped makes this much easier.

Separate fetching data and displaying data

Consider get_local_weather: it has code both to fetch the data and to display the data. I recommend breaking that into two functions. This will make it easier to test your code. For example, you can write a test for fetching the data without requiring that the UI actually be created, and you can test the UI with some test data without having to actually fetch it.

Create two functions: one that fetches the data and returns a dictionary, and then write a second function that takes the dictionary and updates the display:

def get_local_weather(self):
    data = self.fetch_data()
    self.update_ui(data)
\$\endgroup\$
3
\$\begingroup\$

A few more short comments:

  1. Don't compare to None using equality testing. This might bite you in the future (you might have overridden the __eq__ method to always return True for example). Instead compare using the identity comparison is. This works because None is a singleton in Python (there exists only one None object, so all Nones are the same.

    So use if self.temperature is not None: instead of if self.temperature != None:

  2. You should probably use Python 3. Python 2 will become obsolete on January 1st, 2020. All new code should be written in Python 3.

  3. Even in Python 2, % string formatting is the old way to do things. You should use the more modern str.format. This becomes especially nice with the f-string introduced in Python 3.6 (so you have one more reason to switch).

    So instead of "%s%.2f%s" % ("Feel's like ", float(str(data['field4'])), degree_sign) use "Feel's like {:.2f}{}".format(data['field4'], degree_sign).

    Also, is the float(str(...)) call really needed?

  4. Since you are already importing the requests module, why not use it? With your current urllib2 way to retrieve the weather, it is not guaranteed that the connection is actually closed (since an error between opening and closing would terminate the program).

    import requests
    
    class Weather(Frame):
        def __init__(self, parent, *args, **kwargs):
            ...
            self.session = requests.Session()
            # This stuff does not change
            self.url = "http://api.thingspeak.com/channels/%s/feeds/last.json" % CHANNEL_ID
            self.params = {"api_key": READ_API_KEY}
    
        def get_local_weather(self):
            try:
                ...
                response = self.session.get(self.url, params=self.params)
                response.raise_for_status()    # make sure that request was successfull
                data = response.json()    # Already returns a dict
                ...
    

    This has the added benefit that it reuses the connection to the server if possible, speeding up the request.

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