Jump to content

Any things I can improve on my code?

38034580
from tkinter import * #init
from datetime import datetime
from tkinter.ttk import Progressbar
from tk_tools import *
import pygame
from tkinter import messagebox
pygame.mixer.init()
channel=pygame.mixer.find_channel()
alarmchannel=pygame.mixer.find_channel()
from tkinter.ttk import *
from tkinter import filedialog
import time
clock=Tk()
hour=0
clock.barmode=False
clock.strike=False
mins=0
secs=0
clock.settingalarm=False
def demo_chime(): #demos chime sound
    if channel.get_busy()==False and clock.chime.get()>0: #checks if chime setting is on
        try:
            channel.play(pygame.mixer.Sound('00.wav'))
        except:
            messagebox.showerror('Error', 'File 00.wav cannot be found.')
def play_chime():
    if channel.get_busy()==False:
        if clock.chime.get()==1 and mins=='00': #checks chime setting and minute value, if true, chime is set to "On" mode
            if not clock.strike: #check if striking hours is enabled
                try:
                    channel.play(pygame.mixer.Sound('00.wav'))
                except:
                    messagebox.showerror('Error', 'File 00.wav cannot be found.')
            else:
                try:
                    channel.play(pygame.mixer.Sound(hour+'.wav'))
                except:
                    messagebox.showerror('Error', 'File '+hour+'.wav cannot be found.')
        elif clock.chime.get()==2: #4x4 mode
            if clock.strike and mins=='00':
                try:
                    channel.play(pygame.mixer.Sound(hour+'.wav'))
                except:
                    messagebox.showerror('Error', 'File '+hour+'.wav cannot be found.')
            else:
                try:
                    channel.play(pygame.mixer.Sound(mins+'.wav'))
                except:
                    messagebox.showerror('Error', 'File '+mins+'.wav cannot be found.')
def set_alarm():
    errorLabel.config(text='')
    if not clock.settingalarm:
        clock.settingalarm=True
        clock.alarm.set(0)
        hourEntry.config(state='readonly')
        minuteEntry.config(state='readonly')
        offButton.config(state=DISABLED)
        onButton.config(state=DISABLED)
    else:
        if hourEntry.get()=='' or minuteEntry.get()=='':
            errorLabel.config(text='Error: Blank values!')
        else:
            clock.settingalarm=False
            clock.alarmhour=int(hourEntry.get())
            clock.alarmmins=int(minuteEntry.get())
            hourEntry.config(state=DISABLED)
            minuteEntry.config(state=DISABLED)
            offButton.config(state=NORMAL)
            onButton.config(state=NORMAL)
def stop_alarm():
    alarmchannel.stop()
    setButton.config(text='Set Alarm', command=set_alarm)
clock.config(bg='black')
clock.resizable(False, False)
clock.title('Tick v2.0 ©sserver')
day_percent=DoubleVar()
hourLabel=SevenSegmentDigits(clock, background='black', digit_color='#00ff00', height=100, digits=2)
hourLabel.grid(column=1, row=1)
colon1=Label(clock, text=':', foreground='white', background='black', font=('Century Gothic', 75))
colon1.grid(column=2, row=1)
minuteLabel=SevenSegmentDigits(clock, background='black', digit_color='#00ff00', height=100, digits=2)
minuteLabel.grid(column=3, row=1)
colon2=Label(clock, text=':', foreground='white', background='black', font=('Century Gothic', 75))
colon2.grid(column=4, row=1)
secondLabel=SevenSegmentDigits(clock, background='black', digit_color='#00ff00', height=100, digits=2)
secondLabel.grid(column=5, row=1)
Progressbar(clock, mode='determinate', variable=day_percent, length=350).place(x=10, y=130)
clock.chime=IntVar()
clock.alarm=IntVar()
clock.lift()
clock.alarmhour=0
clock.alarmmins=0
Radiobutton(clock, 
               text="Off",
               variable=clock.chime, 
               value=0, command=channel.stop).place(x=20, y=160)
Radiobutton(clock, 
               text="On",
               variable=clock.chime, 
               value=1).place(x=60, y=160)
Radiobutton(clock, 
               text="4x4",
               variable=clock.chime, 
               value=2).place(x=98, y=160)
Label(clock, text='↑Chime Options                                               Alarm Options↓', background='black', foreground='white').place(x=20, y=190)
errorLabel=Label(clock, background='black', foreground='red', text='')
errorLabel.place(x=120,y=190)
clock.mode=False
clock.keepontop=False
clock.geometry('370x250')
playButton=Button(clock, text='Play (Hour) Chime', command=demo_chime)
offButton=Radiobutton(clock, 
               text="Off",
               variable=clock.alarm, 
               value=0, command=stop_alarm, state=DISABLED)
offButton.place(x=20, y=220)
onButton=Radiobutton(clock, 
               text="On",
               variable=clock.alarm, 
               value=1, state=DISABLED)
onButton.place(x=60, y=220)
setButton=Button(clock, text='Set Alarm', command=set_alarm)
setButton.place(x=260, y=220)
hourEntry=Spinbox(clock, from_=0, to=23, width=10, state=DISABLED)
hourEntry.place(x=100, y=220)
minuteEntry=Spinbox(clock, from_=0, to=59, width=10, state=DISABLED)
minuteEntry.place(x=180, y=220)
playButton.place(x=250, y=160)
clock.options_open=False
def close():
    clock.destroy()
    if clock.options_open:
        clock.options.destroy()
clock.protocol('WM_DELETE_WINDOW', close)
def reset():
    clock.options_open=False
    clock.options.destroy()
def options():
    def update_values():
        if chk.instate(['selected']):
            clock.mode=True
        else:
            clock.mode=False
        if chk1.instate(['selected']):
            clock.keepontop=True
        else:
            clock.keepontop=False
        if chk2.instate(['selected']):
            clock.strike=True
        else:
            clock.strike=False
    def barmode(n):
        if str(rb.get())=='Part of hour':
            clock.barmode=True
        else:
            clock.barmode=False
    if not clock.options_open:
        clock.options=Tk()
        clock.options.title('Options')
        chk=Checkbutton(clock.options, command=update_values, text='Show part of day/hour remaining in bar')
        chk.pack()
        chk1=Checkbutton(clock.options, command=update_values, text='Keep on top')
        chk1.pack()
        chk2=Checkbutton(clock.options, command=update_values, text='Strike hours')
        chk2.pack()
        Label(clock.options, text='What do you want to show on the progress bar?').pack()
        rb=Combobox(clock.options, values=['Part of day', 'Part of hour'], state='readonly')
        rb.bind("<<ComboboxSelected>>", barmode)
        rb.pack()
        chk.state(['!alternate'])
        chk1.state(['!alternate'])
        chk2.state(['!alternate'])
        if clock.mode:
            chk.state(['selected'])
        if clock.keepontop:
            chk1.state(['selected'])
        if clock.strike:
            chk2.state(['selected'])
        if clock.iconpresent:
            clock.options.iconbitmap('clock.ico')
        if clock.barmode:
            rb.set('Part of hour')
        else:
            rb.set('Part of day')
        clock.options.protocol('WM_DELETE_WINDOW', reset)
        clock.options.resizable(False, False)
        clock.options_open=True
    else:
        clock.options.lift()
Button(clock, text='Options', command=options).place(x=160, y=160)
clock.iconpresent=True
try:
    clock.iconbitmap('clock.ico')
except:
    messagebox.showerror('Icon Error', 'App icon is missing or corrupt')
    clock.iconpresent=False
while True:
    try:
        d = datetime.now()
        hour = d.strftime("%I")
        hour2=d.strftime('%H')
        mins = d.strftime("%M")
        secs = d.strftime("%S")
        day =  d.strftime("%A")
        part_of_day=d.strftime("%p")
        hourLabel.set_value(hour)
        minuteLabel.set_value(mins)
        secondLabel.set_value(secs)
        clock.update()
        if clock.keepontop:
            clock.call('wm', 'attributes', '.', '-topmost', True)
        else:
            clock.call('wm', 'attributes', '.', '-topmost', False)
        if not clock.barmode:
            if clock.mode:
                day_percent.set(((86400-((int(hour2)*3600+int(mins)*60+int(secs))))/86400)*100)
            else:
                day_percent.set(((int(hour2)*3600+int(mins)*60+int(secs))/86400)*100)
        else:
            if clock.mode:
                day_percent.set(((3600-(int(mins)*60+int(secs)))/3600)*100)
            else:
                day_percent.set(((int(mins)*60+int(secs)))/3600*100)
        if clock.alarm.get()==1 and (int(mins)==clock.alarmmins and int(hour2)==clock.alarmhour and secs=='00') and (not alarmchannel.get_busy()):
            try:
                alarmchannel.play(pygame.mixer.Sound('alarm.wav'), -1)
                setButton.config(text='Stop Alarm', command=stop_alarm)
            except:
                messagebox.showerror('Error', 'File alarm.wav cannot be found.')
        if (mins=='00' or mins=='15' or mins=='30' or mins=='45') and secs=='00':
            play_chime()
        time.sleep(0.01)
    except:
        break

Can I improve in anything in this code? It's a clock that chimes and strikes the hours. Note that files 01-12.wav are hour chimes with striking, while 00.wav is the hour chime without striking. 15, 30, and 45.wav are quarter-hour chimes. The chime "On" mode sets chimes hourly while 4x4 sets chimes quarter-hourly.

Link to comment
Share on other sites

Link to post
Share on other sites

From a pure code perspective I would say reformat it a bit:

  • Follow a style-guide, for example PEP-8, Google style, Numpy style
  • Related to that: add docstrings to your functions explaining what they do and what arguments they take
  • Add comments if some code is not immediately obvious and you will have forgotten what it is for next month.

After a quick glance, multiple calls to things like

if clock.chime.get()==1

can be consolidated into a single call

current_chime = clock.chime.get()
if current_chime == 1:

The minutes

if (mins=='00' or mins=='15' or mins=='30' or mins=='45') and secs=='00':

If you store them as a number you can exploit the remainder and simply check if they are divisable by 15:

if not (mins % 15) and secs == '00':

and since  you are calling

int(hour2)*3600+int(mins)*60+int(secs)

so often, that should become it's own function that takes hours, minutes and seconds as input and returns the result.

 

Crystal: CPU: i7 7700K | Motherboard: Asus ROG Strix Z270F | RAM: GSkill 16 GB@3200MHz | GPU: Nvidia GTX 1080 Ti FE | Case: Corsair Crystal 570X (black) | PSU: EVGA Supernova G2 1000W | Monitor: Asus VG248QE 24"

Laptop: Dell XPS 13 9370 | CPU: i5 10510U | RAM: 16 GB

Server: CPU: i5 4690k | RAM: 16 GB | Case: Corsair Graphite 760T White | Storage: 19 TB

Link to comment
Share on other sites

Link to post
Share on other sites

Check for the presence of the wav files at the beginning. Optionally, If not present, generate them (or unpack them from somewhere)

Have all the strings as constants or in a configuration / xml / json file and retrieve the string you need with a function. this would make it easier to quickly find some text you want to edit and edit it, and maybe offer multi language interface. 

Isn't messagebox blocking? So if you get one messagebox everything stops? Or do you get multiple message boxes generated?

 

Link to comment
Share on other sites

Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

×