I have some code that asks for a user input, and then uses TTS to convert that into speech and play the file. It then asks the user whether they want to repeat it and convert another input into speech, or whether they want to exit the program. Are there any improvements that I could make to the code?
import webbrowser
import os
import time
import sys
import getpass
from gtts import gTTS
from mutagen.mp3 import MP3
my_file = "C:/Users/%USERNAME%/Desktop/TTS/bob.mp3" #Sets a variable for the file path.
username = getpass.getuser() #Gets the username of the current user.
def repeat():
while True:
if os.path.isfile(my_file): #Checks to see whether there is a file present and, if so, removes it.
os.remove(my_file)
else:
None
tts = gTTS(text = input("Hello there " + username + """. This program is
used to output the user's input as speech.
Please input something for the program to say: """)) #Takes the user's input and uses it for the Text-To-Speech
tts.save('bob.mp3') #Saves a .mp3 file of the user's input as speech.
webbrowser.open(my_file) #Opens the .mp3 file
audio = MP3(my_file) #Sets a variable so that the Mutagen module knows what file it's working with.
audio_length = audio.info.length #Sets a variable of the length of the .mp3 file.
time.sleep((audio_length) + 0.25) #Waits until the file has finished playing.
os.system('TASKKILL /F /IM wmplayer.exe') #Closes Windows Media Player.
time.sleep(0.5) #Waits until Windows Media Player has closed.
while True:
answer = input("Do you want to repeat? (Y/N) ")
if answer == "y" or answer == "Y":
return repeat() #Goes back to the beginning of the function if the user wants to try again.
elif answer == "n" or answer == "N":
if os.path.isfile(my_file): #Checks to see whether there is a file present and, if so, removes it.
os.remove(my_file)
else:
None
sys.exit() #Exits the program.
else:
print("Sorry, I didn't understand that. Please try again with either Y or N.")
continue #Goes back to the beginning of the while loop.
repeat() #Calls the function.
2 Answers 2
DRY
Apply the DRY (Do not Repeat Yourself) method. For instance, the check to remove the file. Since it's being called in two places, we can make this a function. I've re-written your code in a more functional style. This is also easier to read what's occurring.
else
Your else: None
statements are not doing anything.
continue
Same goes for this in the while
loop. This happens anyway.
if
When you check the user's answer, just assume it's going to be varying. Instead of checking for an UPPERCASE 'y' and lowercase, just lower it. Also stripping it to make sure to remove erroneous spaces.
answer = input("Do you want to repeat? (Y/N) ").strip().lower()
if answer in ['yes', 'y']:
# instead of
answer = input("Do you want to repeat? (Y/N): ")
if answer == "y" or answer == "Y":
Exceptions
Since you're interacting with the user from the interpreter, you can catch if the user exits the program and run any cleanup code before cleanly exiting. I've wrapped the loop of the interaction in a try/except statement so that this can be caught.
if name == 'main':
Check this SO post out.
Rewritten
import webbrowser
import os
import time
import getpass
from gtts import gTTS
from mutagen.mp3 import MP3
my_file = "C:/Users/%USERNAME%/Desktop/TTS/bob.mp3" #Sets a variable for the file path.
username = getpass.getuser() #Gets the username of the current user.
def remove_file():
"""Checks if myfile exists and if so, deletes."""
if os.path.isfile(my_file):
os.remove(my_file)
def play_tts():
webbrowser.open(my_file) # Opens the .mp3 file
audio = MP3(my_file) # Sets a variable so that the Mutagen module knows what file it's working with.
audio_length = audio.info.length # Sets a variable of the length of the .mp3 file.
time.sleep(audio_length + 0.25) # Waits until the file has finished playing.
os.system('TASKKILL /F /IM wmplayer.exe') # Closes Windows Media Player.
time.sleep(0.5) # Waits until Windows Media Player has closed.
def ask_and_play():
# Takes the user's input and uses it for the Text-To-Speech
tts = gTTS(text=input(
"Hello there " + username + ". This program isn\n"
"used to output the user's input as speech.\n"
"Please input something for the program to say: ")
)
tts.save('bob.mp3') # Saves a .mp3 file of the user's input as speech.
play_tts()
def check_continue():
"""Checks if the user wants to continue.
Returns a boolean value."""
while True:
answer = input("Do you want to repeat? (Y/N) ").strip().lower()
if answer in ['yes', 'y']:
return True
elif answer in ['no', 'n']:
return False
else:
print("Sorry, I didn't understand that. Please try again with either Y or N.")
def repeat():
"""Repeatedly ask the user to type text to play,
and check if the user wants to repeat or exit."""
while True:
remove_file()
ask_and_play()
if not check_continue():
raise KeyboardInterrupt
if __name__ == '__main__':
try:
repeat() #Calls the function.
except KeyboardInterrupt:
# clean up
remove_file()
print('Goodbye!')
Comments
A core rule of comments is that they add to code, by explaining why you're doing something the way you are. A bad way to use comments is by just repeating what the code does. For example:
my_file = "C:/Users/%USERNAME%/Desktop/TTS/bob.mp3" #Sets a variable for the file path.
The comment is useless. A better choice in this case, would be:
file_path = "C:/Users/%USERNAME%/Desktop/TTS/bob.mp3"
Without a comment. Another example:
os.path.isfile(my_file): #Checks to see whether there is a file present and, if so, removes it.
the names isfile()
and remove()
are pretty good indications of what the functions do, so a comment is definitely not needed in that case.
If-else
else:
None
Isn't good practice. Much better would be to use
else:
pass
Or even better, leave the else
statement out.
Bad variable names
Names like repeat()
are not good for functions, they don't explain what happens. A name like text_to_speech()
would be more descriptive.
Improved code
Here's my version of your code:
import webbrowser
import os
import time
import getpass
from gtts import gTTS
from mutagen.mp3 import MP3
FILE_PATH = "C:/Users/%USERNAME%/Desktop/TTS/bob.mp3"
username = getpass.getuser()
def text_to_speech():
while True:
if os.path.isfile(my_file):
os.remove(my_file)
tts = gTTS(text = input("Hello there " + username + """. This program is
used to output the user's input as speech.
Please input something for the program to say: """))
tts.save('bob.mp3')
webbrowser.open(my_file)
audio = MP3(my_file)
audio_length = audio.info.length
time.sleep((audio_length) + 0.25) # Wait until the mp3 file has finished
os.system('TASKKILL /F /IM wmplayer.exe')
time.sleep(0.5)
while True:
answer = input("Do you want to repeat? (Y/N): ")
if answer == "y" or answer == "Y":
repeat()
elif answer == "n" or answer == "N":
if os.path.isfile(my_file):
os.remove(my_file)
exit()
else:
print("Sorry, I didn't understand that. Please try again with Y or N.")
text_to_speech()
Some changes I made:
Removed unnecessary comments
Removed
sys
import, replacedsys.exit()
withexit()
Added whitespace to make code easier on the eyes
repeat()
is now calledtext_to_speech()
-
1\$\begingroup\$ Thanks! I've taken all of your suggestions and implemented them into the code. The only thing I didn't change is
sys.exit()
toexit()
;exit()
asks for confirmation before closing the program (as the program is still running) whereassys.exit()
does not. Also, in that code you've left the call to the function asrepeat()
instead oftext_to_speech()
. Just thought I'd point that out. :) \$\endgroup\$Foxes– Foxes2017年05月26日 20:46:36 +00:00Commented May 26, 2017 at 20:46 -
\$\begingroup\$ Your'e welcome. If you can, be sure to implement double_j's main() function and call main() for good practice. Also, if you want to use sys.exit(), instead of importing all of sys, use -from sys import exit- \$\endgroup\$Daniel– Daniel2017年05月26日 20:48:02 +00:00Commented May 26, 2017 at 20:48