2
\$\begingroup\$

I created a function which takes some inputs and builds a specific set of strings. The function is working as intended, however I can't help thinking how ridiculous it looks every time I see it. I originally had a more simple way of building the strings using multiple different functions, however I couldn't stand how much repetition there was between them.

This is what I currently have.

def format_command_string(vmrunDir, command, vmxfile="", username="", password="", script="", parameters=[]):
    escapedVmxfile = "\"{}\"".format(vmxfile)
    escapedVmrunDir = "\"{}\"".format(vmrunDir)
    parameters = " ".join(parameters) if parameters else " "
    subStringTuple = (
        escapedVmrunDir,  # 0
        "-T ws",  # 1
        "-gu {}".format(username),  # 2
        "-gp {}".format(password),  # 3
        command,  # 4
        escapedVmxfile,  # 5
        "cmd.exe \"/c Z:\\vm_controller_share\scripts\{}\"".format(script),  # 6
        parameters)   # 7

    formatDictionary = {
        "list": "{0} {4}",
        "runProgramInGuest": "{0} {1} {2} {3} {4} {5} {6} {7}",
        "addSharedFolder": "{0} {1} {2} {3} {4} {5} {7}",
        "removeSharedFolder": "{0} {1} {2} {3} {4} {5} {7}",
        "start": "{0} {1} {4} {5}",
        "stop": "{0} {1} {4} {5} {7}",
        "reset": "{0} {1} {4} {5} {7}"}

    formattedCommandString = formatDictionary[command].format(*subStringTuple).strip()

    return formattedCommandString

It does the job I want it to do fine, but it just seems perhaps a bit... overly complicated? There is some obvious cons to it around readability and maintainability. However I can't bring myself to go back to what I had originally, which was something like:

def format_list_command_string(vmrunDir):
    return "{} list".format(vmrunDir)


def format_run_command_string(vmrunDir, username, password, vmxfile, script, parameters):
    return "{} -T ws -gu {} -gp {} runProgramInGuest {} cmd.exe \"/c Z:\\vm_controller_share\scripts\{} {}\"".format(vmrunDir, username, password, vmxfile, script, parameters)


def format_add_shared_folder_command_string(vmrunDir, username, password, vmxfile, guestSharedFolder, hostShareFolder):
    return "{} -T ws -gu {} -gp {} addSharedFolder {} {} {}".format(vmrunDir, username, password, vmxfile, guestSharedFolder, hostShareFolder)


def format_remove_shared_folder_command_string(vmrunDir, username, password, vmxfile, guestSharedFolder):
    return "{} -T ws -gu {} -gp {} removeSharedFolder {} {}".format(vmrunDir, username, password, vmxfile, guestSharedFolder)


def format_start_command_string(vmrunDir, vmxfile):
    return "{} -T ws start {}".format(vmrunDir, vmxfile)


def format_stop_command_string(vmrunDir, vmxfile, force):
    return "{} -T ws stop {} {}".format(vmrunDir, vmxfile, force)


def format_resetcommand_string(vmrunDir, vmxfile, force):
    return "{} -T ws reset {} {}".format(vmrunDir, vmxfile, force)

I've only really worked alone, and I've not got a lot of exposure to some simple stuff as building strings so I'm not sure what I expect from this review, but all advice is appreciated.

EDIT: Example of how the returned strings would be used at request of @200_success

def try_vmrun_command(commandString):
    try:
        output = subprocess.check_output(commandString)
    except subprocess.CalledProcessError as error:
        print error.output
    else:
        return output
\$\endgroup\$
  • 2
    \$\begingroup\$ Could you also give us an example of how the resulting command strings are used by the rest of your code? I suspect that there is some other badness going on. \$\endgroup\$ – 200_success Mar 7 '18 at 3:37
  • \$\begingroup\$ @200_success, I'm just using it to run the command currently with the check_output function from the subprocess module. Most of the code I'm using is still in the planning/proof of concept stage for me just now, so I'm not really interested in badness in other areas, the focus of my question is on how to improve the function mentioned in the question. \$\endgroup\$ – user162459 Mar 7 '18 at 20:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy