14
\$\begingroup\$

It's still a working process. There are still things I would like to add, and if you have any critiques or ideas they would be greatly appreciated!

I would like to know if my code is readable.

GitHub link:

import os
import webbrowser
import subprocess
import speech_recognition as sr
import json
from time import sleep
from difflib import get_close_matches

# defining functions that are used
def reload_files():
    """ Order is: files, directories,then paths."""
    found_files = []
    found_dirs = []
    found_paths = []
    for paths, dirs, files in os.walk(os.getcwd()):
        found_files += files
        found_dirs += dirs
        found_paths += paths
    return found_files, found_dirs, found_paths

def find_path(file):
    for path, dirs, files in os.walk(os.getcwd()):
            for f in files:
                if f == file:
                    return os.path.join(path, file)
            for d in dirs:
                if d == file:
                    return os.path.join(path, file)

def file_search(file):
    return get_close_matches(file, current_files, cutoff=0.4)[0]

def dir_search(directory):
    try:
        requested_dir = get_close_matches(directory, current_dirs, cutoff=0.4)[0]
        dir_list = []
        # Iterating through the directory that was declared before.
        for dirs in os.listdir(find_path(requested_dir)):
            if os.path.isdir(find_path(dirs)):
                dir_list.append("{}/{}".format(requested_dir, dirs))
        if len(get_close_matches(directory, dir_list, cutoff=0.4)) == 0:
            return find_path(requested_dir)
        else:
            return get_close_matches(directory, dir_list, cutoff=0.4)[0]
    except:
        print('Could not find directory "{}"!'.format(directory))

def exe(command):
    duckurl = "https://www.duckduckgo.com/?q="
    command = command.lower()
    order = command.split()[0]
    if order == "search":
        website = command.split()[1]
        # if command.split()[1] in websites then it will search the website
        # instead of googling it.
        if website in websites:
            webbrowser.open("{}{} {}".format(duckurl, websites[website], command[command.index(command.split()[2]):]))
            return True
        else:
            webbrowser.open(duckurl + command[command.index(command.split()[1]):])
            return True

    elif "play directory" in command:
        combined_dir =  dir_search(command[command.index(command.split()[2]):])
        for paths, dirs, files in os.walk(os.getcwd()):
            if combined_dir in paths:
                os.system("vlc {}".format(paths))
                return True

    elif order == "play":
        #this will play movies
        file = file_search(command[command.index(command.split()[1]):])
        subprocess.call(["xdg-open",find_path(file)])
        return True

    elif order == "run":
        os.system(command[command.index(command.split()[1]):])
        return True
    elif order == "add":
        # The second letter will be the webbsite third will be the bang
        website_bang = {command.split()[1]: "!" + command.split()[2]}
        websites.update(website_bang)

        with open("websites.json", "w") as websites_json:
            json.dump(websites, websites_json)

        print(websites)
        return True
    elif order == "refresh" or order == "reload":
        print("{}ing files...".format(order.capitalize()))
        global current_files, current_dirs, current_subs
        current_files, current_dirs, current_subs = reload_files()
        print("Complete!")
        return True
    else:
        print('Could not recognize command "{}"!'.format(command))
        return False

# speech recognition
def listening():
    with sr.Microphone() as sauce:
        print("Listening...")
        try:
            audio = r.listen(sauce, timeout=5)
        except sr.WaitTimeoutError:
            print("You ran out of time!")
            return False
    print("Recognizing...")
    try:
        command_worked = exe(r.recognize_google(audio))
    except sr.RequestError:
        print("Something went wrong with the conection. Trying sphinx...")
        command_worked = exe(r.recognize_sphinx(audio))
    except sr.UnknownValueError:
        print("Could not hear what you were saying!")
        return False
    if not command_worked:
        print("Something went wrong with the exe function!")
        return False
    else:
        return True
# Constants
current_files, current_dirs, current_subs = reload_files()
r = sr.Recognizer()
with open("websites.json") as websites_json:
    websites = json.load(websites_json)

while True:
    listening()
\$\endgroup\$

2 Answers 2

20
\$\begingroup\$

This is very interesting. I love it. Good work.


Critcism:

   with sr.Microphone() as sauce:

Sauce

From https://pixabay.com/photos/tomato-soup-tomato-soup-sauce-482403/

  • Sauce is what you add to pasta. Microphone is a source.
def exe(command):
    duckurl = "https://www.duckduckgo.com/?q="
  • This function is too long and should be shortened.
  • We can use a dictionary of commands and callables. (Callables are functions, lambdas, classes, objects with __call__ implementation, ...)
  • If play directory is different you can either iterate the dictionary (In that case you might as well use a list) or handle it as a special case.
# speech recognition
def listening():
  • Why not just rename to recognize_speech?
from time import sleep
  • Delete this. Is this used?
# defining functions that are used 
  • This comment is not needed.
r = sr.Recognizer()
  • Can we rename this to RECOGNIZER or something meaningful.
def find_path(file):
    for path, dirs, files in os.walk(os.getcwd()):
            for f in files:
       # ^^^^^^
       # over indent  
  • Nested for is over-indented. (This might also be an error of copying to code-review, so check this too)
current_files, current_dirs, current_subs = reload_files()
r = sr.Recognizer()
with open("websites.json") as websites_json:
    websites = json.load(websites_json)
  • I think it is better to move the constants to the top of the file, so we know where they come from. I would also recommend making them SCREAMING_SNAKE_CASE to highlight they are constants.
  • Small caveat: you might need to add reload_files to top of the file before calling it or import it.
while True:
    listening()
  • It would be better to use if __name__ == "__main__" idiom. This way you can use this code as a module as well.
  • You can also catch keyboard inturrupt and exit gracefully.

Ideas/Feature Requests:

These are improvements that I think you can add to your code.

  • print('Could not find directory "{}"!'.format(directory)) - replace print with speech.
  • create a help command.
\$\endgroup\$
10
  • \$\begingroup\$ Are you saying have a sperate function for each command? \$\endgroup\$ Commented Dec 19, 2019 at 21:18
  • 1
    \$\begingroup\$ I'd recommend that. So you can add all the commands and callables to a dict. It's a python idiom . \$\endgroup\$
    – JaDogg
    Commented Dec 19, 2019 at 21:48
  • 1
    \$\begingroup\$ I am not fond of functions using global variables; is it really good practice to refer in functions to global variables defined somewhere way below function body (referring to "r") \$\endgroup\$
    – Gnudiff
    Commented Dec 20, 2019 at 7:46
  • 1
    \$\begingroup\$ @409_Conflict generally trying to be cute with variable names is confusing to people from a different culture. \$\endgroup\$
    – JaDogg
    Commented Dec 20, 2019 at 10:23
  • 1
    \$\begingroup\$ @moopet I've specified it before.. I don't think my criticism points are in order. \$\endgroup\$
    – JaDogg
    Commented Dec 20, 2019 at 12:46
4
\$\begingroup\$

Just some small things:

find_path could be neatened up a bit using any and chain from itertools:

from itertools import chain

def find_path(file):
    for path, dirs, files in os.walk(os.getcwd()):
        if any(x == file for x in chain(files, dirs)):
            return os.path.join(path, file)

    return None

chain essentially lets you lazily concatenate any iterables. If files and dirs are lists, and you don't mind a little extra overhead, you could just replace that bit with:

any(x == file for x in files + dirs)

A thing to note though, it's possible for find_path to return None in your current code! This will cause TypeError: 'NoneType' object is not iterable and similar errors. You may want to ensure that it returns something valid before you use it. This can be done neatly using Python 3.8's assignment expression:

if found := find_path(. . .):
    print("Path was found")

else:
    print("find_path returned None")

Regardless though, if you want to allow a function to return None, you should explicitly return None instead of relying on the default behavior.


In multiple places, you write basically this:

get_close_matches(#, #, cutoff=0.4)

If you ever decide that you want to change the 0.4 cutoff, you need to track down all the places that it's used. Don't use magic numbers. Instead, define that as a constant and use the constant:

STANDARD_CUTOFF = 0.4

. . .

get_close_matches(directory, dir_list, cutoff=STANDARD_CUTOFF)

Now, you only need to change STANDARD_CUTOFF in the one place to have the change happen everywhere.


In dir_search you have at the bottom:

except:
    print('Could not find directory "{}"!'.format(directory))

This is a problem. The try is wrapping essentially eight lines of dense code, and you're using a catch-all except. What if you accidentally inserted an error in there when editing the code? You'll get 'Could not find directory "{}"!' messages that mask the real error that you're getting. Specify the exact exception you want to catch so you don't accidentally silence actual errors. I'm assuming you want:

except IOError:
    print('Could not find directory "{}"!'.format(directory))

if order == "search":
    website = command.split()[1]

    if website in websites:
        webbrowser.open("{}{} {}".format(duckurl, websites[website], command[command.index(command.split()[2]):]))
        return True
    else:
        webbrowser.open(duckurl + command[command.index(command.split()[1]):])
        return True

Notice how both the branches there end in a return True. That can just be taken out:

if order == "search":
    website = command.split()[1]

    if website in websites:
        webbrowser.open("{}{} {}".format(duckurl, websites[website], command[command.index(command.split()[2]):]))
    else:
        webbrowser.open(duckurl + command[command.index(command.split()[1]):])

    return True

You could try getting rid of the duplicated webbrowser.open call too. Maybe:

if website in websites:
    url = "{}{} {}".format(duckurl, websites[website], command[command.index(command.split()[2]):])

else:
    url = duckurl + command[command.index(command.split()[1]):]

webbrowser.open(url)

return True

If the lines defining url were shorter, you could use a conditional expression here too and avoid the double definitions. That doesn't work well here unfortunately though.


elif order == "refresh" or order == "reload":

This can be neatened up a bit using in:

elif order in {"refresh", "reload"}:

I'm using a set, but in a case like this, a list would work fine as well.


I agree with @bhathiya-perera though about exe. It's far too long, and having it as a series of order == has two issues:

  • It makes adding commands more difficult. Any time you want to add a new command, you need to modify a central function. Ideally, you should be modifying code as little as possible once it's pretty, tested and working. The less opportunities you have to add bugs to code, the better.

  • It will be a trivial difference, but the commands lower in the elif checks will take longer, because each time, you needed to check all the commands before it. I think a dictionary maping strings to functions would be a much cleaner long term solution, and would potentially allow users of your library to add their own commands (without needing to edit your source).

\$\endgroup\$

Your Answer

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

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