5
\$\begingroup\$

My script presents an options menu for scripts contained within the "options" folder. The user selects the scripts they want to run, and enters "start". This then starts the scripts in a specific order (not included) - the order the scripts are in the options menu.

I believe it can be a lot shorter, more appealing and Pythonic. How can I improve it?

import os
import xml.etree.ElementTree as ET
import sys
import customError
import login

class optionMenu():
    __slot__ = ["options"]
    def __init__(self):
         #creates a dictionary of usable programs and if they are selected or not
        self.options = {k.split(".")[0].lower():False for k in os.listdir("Options") if k.endswith(".exe")}
    def menu(self): #3 when called displays GUI
        print("\n".join(["[x] "+name if bool == True else "[ ] "+name for name,bool in self.options.items()]))
    def select(self,selection):
        if(selection.lower() == "start" and True not in self.options.values()):
            print("Can't Start")
        elif(selection.lower() == "start" and True in self.options.values()):
            print("starting",",".join([name for name,bool in self.options.items() if bool == True]))
            return False
        elif(selection.lower() not in self.options.keys()):
            print("Please make a valid selection")
        elif(self.options[selection.lower()] == False):
            self.options[selection.lower()] = True
        return True
    def main(self):
        self.menu()
        choice = input("Choice: ")
        return self.select(choice)
# End of OptionMenu Class
class scripts():
    __slot__ = ["tree","root","script","version","scriptName","author","contributors","scriptType","executable"]
    def __init__(self,script):
        self.version = script.find('version').text
        self.scriptName = script.find('scriptName').text
        self.author = script.find('author').text
        self.contributors = self.adjustcon([script.find('contributors').text])
        self.scriptType = script.find('scriptType').text
        self.executable = os.path.join("Options",self.scriptName,".exe")
    def adjustcon(self,contributors):
        if(len(contributors) > 1): #20 checks if selected list is greater than 1 element
            return (", ".join(contributors[:-1]) ,"and",contributors[-1]) #21 formats output -> Running: element,..., and element
        elif(contributors[0] == None):
            return None
        else: #22 if only one selection made
            return (", ".join(contributors))
    def getVersion(self):
        return self.version
    def setVersion(self,version):
        self.version = version
    def getName(self):
        return self.scriptName
    def setName(self,name):
        self.scriptName = name
    def getAuthor(self):
        return self.author
    def setAuthor(self,author):
        self.author = author
    def getContributors(self):
        return self.contributors
    def setContributors(self,contributors):
        self.contributors = contributors
    def getType(self):
        return self.scriptType
    def setTypes(self,type):
        self.scriptType = type
    def getExecutable(self):
        return self.executable
    def setExecutable(self,exe):
        self.executable = exe

def getScripts():
    scriptDict = {k.find('scriptName').text:scripts(k) for k in ET.parse("scriptInfo.xml").getroot().findall('script')}
    return scriptDict


def openingScreen(scripts): #19 formatting for to part that appears.
    script = next(value for key,value in scripts.items() if value.scriptType=="optionMenu")
    scriptName = script.scriptName
    welcome = "{0} WELCOME TO {1} {0}".format("-"*48,scriptName)
    credit = welcome+"""\nVersion: {0}\nDeveloped by {1} With help from {2}
{3} DISCLAIMERS {3}\nVerify that all usernames and password entered are valid. If the script needs to be terminated press ctrl+C.
Select all needed programs, multitool will run them in proper order. Once complete the respective notes/logs will be stored in a folder.\n{4}""".format(script.version,script.author,script.contributors
                                                                                                                                                    ,"-"*int((len(welcome)-13)/2),"-"*len(welcome))
    return (credit,scriptName)

if __name__ == "__main__":
    try:
        scripts = getScripts()
        screen,currentScript = openingScreen(scripts = scripts)
        menu = optionMenu()
        deciding = True
        while deciding:
            os.system('cls||clear') # clears cmd for illusion of updating
            print(screen)
            deciding = menu.main()
        # need to add method by which to pass variables.
        lines = open("RunOrderList.txt","r")
        for line in lines:
            print(line)
        login.login()
    except KeyboardInterrupt: # catch exit command ctrl+C
        print("Exiting {0}".format(currentScript))
        input("Press the enter key to continue...")
    except Exception as e: # Catches Unexpected exceptions
        exc_type, exc_obj, exc_tb = sys.exc_info()
        fname = os.path.split(exc_tb.tb_frame.f_code.co_filename)[1]
        print(exc_type, fname, exc_tb.tb_lineno)
```
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

Overall comment

Your main goal should not be to do as much as possible with as few lines of code as possible, even if you can and even if Python expressions make it possible.

Suggestion 0: Code style and formatting

Read a Python style guide ( https://www.python.org/dev/peps/pep-0008/ ) and run your code through an autoformatter to fix the style quickly. Some space between function definitions is one thing that would help readability.

https://www.tutorialspoint.com/online_python_formatter.htm

You don't need parentheses when using if, that's done in Java and many other languages but not Python.

Suggestion 1: Readability and maintainability

self.options = {k.split(".")[0].lower():False for k in os.listdir("Options") if k.endswith(".exe")}

Code like this is difficult to read.

Start by splitting your code up into stepwise instructions so that it can be parsed by a human. Don't worry about the number of lines.

self.options = {}

for k in os.listdir("Options"):
    if k.endswith(".exe"):
        filename = k.split(".")[0].lower()
        self.options[filename] = False

Suggestion 2: Bloat(?)

def getVersion(self):
        return self.version
    def setVersion(self,version):
        self.version = version
    def getName(self):
        return self.scriptName
    def setName(self,name):
        self.scriptName = name
    def getAuthor(self):
        return self.author

You don't need setters and getters. Just delete them and access your variables directly. There may be programs or situations where it makes sense to have setters and getters, but this is not one.

Suggestion 3: Don't repeat yourself

https://dzone.com/articles/software-design-principles-dry-and-kiss

(and more)

Starting here

def select(self,selection):
    if(selection.lower() == "start" and True not in self.options.values()):
        print("Can't Start")
    elif(selection.lower() == "start" and True in self.options.values()):
        print("starting",",".join([name for name,bool in self.options.items() if bool == True]))
        return False
    elif(selection.lower() not in self.options.keys()):
        print("Please make a valid selection")
    elif(self.options[selection.lower()] == False):
        self.options[selection.lower()] = True
    return True

selection.lower() and self.options are used a lot, let's redefine them once instead.

def select(self,selection):
    selection = selection.lower()
    opts = self.options
    if(selection == "start" and True not in opts.values()):
        print("Can't Start")
    elif(selection == "start" and True in opts.values()):
        print("starting",",".join([name for name,bool in opts.items() if bool == True]))
        return False
    elif(selection not in opts.keys()):
        print("Please make a valid selection")
    elif(opts[selection] == False):
        opts[selection] = True
    return True

The first two checks have a lot in common and belong together, so we can refactor those.

def select(self,selection):
    selection = selection.lower()
    opts = self.options
    if selection == "start":
        if True in opts.values():
            print("starting",",".join([name for name,bool in opts.items() if bool == True]))
            return False
        else:
            print("Can't Start")
            return True
   
    if selection not in opts.keys():
        print("Please make a valid selection")
    elif(opts[selection] == False):
        opts[selection] = True
    return True

This can be improved further but I'm running out of time.

bool is a keyword. Don't name your variables bool.

Use an editor like PyCharm or Visual studio Code or anything else that has syntax highlighting and warning for such errors.

\$\endgroup\$
1
  • \$\begingroup\$ I appreciate you taking the time to look at my code! I always enjoy a chance to learn. If you want to look at the whole project checkout: github.com/michaeldcanady/MultiTool \$\endgroup\$ Sep 16, 2020 at 1:49

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.