HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Refactoring Tkinter GUI that reads from and updates csv files, and opens E-Run files

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
refactoringruncsvtkinterreadsfilesthatandfromgui

Problem

Background

My lab administers four computer programs in three separate appointments. At each appointment, the subject does the programs in a pseudo-randomized order. Also, three of the programs have subversions, which are also pseudo-randomized so that the subject could do type A of program 1 at the first appointment, type C of program 1 at followup 1, and type B of program 1 at followup 2.

The orders and individual program subversions that subjects have run in the past are stored in csv files (one file for each program, and one file for overall program order). The appropriate pseudo-randomized order for each timepoint is stored as a list of lists in a pickle file.

Code Purpose

Basically, my GUI takes in the unique subject ID number and appointment number and looks through the overall order file to determine if the subject has been recorded, and determines, based on the row in which the subject ID occurs (a new row if the subject is new), which order the programs should be given in and which version of each program should be performed. It also updates the csv with that information. Once the GUI determines the appropriate program and subversion to run, it simply opens up the program, waits until the subject has finished with the task, and then executes the next program.

It is supposed to deal with input errors, like overwriting or inputting an unacceptable appointment number, by giving the option to restart or quit gracefully. Ultimately, I will create a shortcut for this script on a Windows computer so that it can be run purely graphically.

Review Goals

I am hoping to make my code as efficient and readable as possible, so any advice is welcomed, but I do have a couple of specific concerns:

-
How can I clean up get_curr_order's inputs? Can I make set_overwrite a global variable?

-
Is there a better way to handle restarting the GUI in get_curr_order?

-
Is there a better way to handle exceptions in my try/except statements? Both exceptions are perfectly

Solution

UI Code

The windows set a response variable as a string. However, in most cases, the response can only be one of two values. Strings like "Yes" and "No" are good when a UI displays something to a user, but a computer works much better with boolean values.

Just because UI code doesn't mean you no longer have to use good descriptive variable names. Don't use things like label_1 or button_2. Using a number in a variable name is rarely the correct thing to do.

class OverwriteButton(Tkinter.Tk):
    """
    Creates a window with query (Do you wish to overwrite) and two buttons, Yes
    and No. Each sets response attribute to respective string.
    """


The class name says it is a button, but the documentation says it is a window. Which one is correct?

You recognized that get_curr_order() is taking a lot of parameters. It is acceptable to make a class that just stores data that can be passed into a function.

However, a better idea would be to break the function into smaller sub-functions. The first thing get_curr_order() does is read text from a file. This function should be concerned with the logic of getting the current order, not reading text from a csv file. A bonus benefit of doing this is now you can test this function without having to write the test data to a file before calling the function you want to test.

There are a number of other sub sections within get_curr_order(). If you find yourself writing a comment that says what you are doing, that is a good indication that the following block of code would be a good section that can be extracted into a function with a descriptive name. Comments are better at saying why some operation is being done.

def safe_name(process):
    """
    Check for names of processes in psutil.process_iter and, if
    permission denied, returns "None".
    """
    try:
        return process.name
    except Exception:  # <-- Which exception can I use to pass without error?
        return "None"


Don't catch Exception. You can test the code to see what exception is actually thrown (AttributeError). Better yet, Python has getattr() for getting an attribute and returning a default if it doesn't exist.

while open_proc is True:
    data = list(psutil.process_iter())
    if any(["gedit" in safe_name(Proc) for Proc in data]):
        open_proc = True
    else:
        open_proc = False


You don't have to explicitly use True or False.

while open_proc:
    data = list(psutil.process_iter())
    open_proc = any(["gedit" in safe_name(proc) for proc in data])


Also note the change to proc. CapitalCase is used for classes.

Taking a step back and looking at the code, you should not be using that loop to wait for the process to terminate. call() blocks until the process terminates. [Popen.wait()][2] will do the same.

Pickling Code

At first glance, it looks like task_order includes all of the permutation 'AX', 'RISE', 'Kirby', and 'Decimal'. Python has permutations() for that instead of writing it out. Using the function will be easier to read and less error prone.

In general, the generation of the pickle files is full with lots of constants with not context. It would be a good idea to include some comments that explain what these values are and how they are being used.

All of the paths are hard coded and absolute. It would be better to either write files to locations relative to the working directory (or at least have a variable specifying the base path). Doing this will make it easier for another person to run the code.

Code Snippets

class OverwriteButton(Tkinter.Tk):
    """
    Creates a window with query (Do you wish to overwrite) and two buttons, Yes
    and No. Each sets response attribute to respective string.
    """
def safe_name(process):
    """
    Check for names of processes in psutil.process_iter and, if
    permission denied, returns "None".
    """
    try:
        return process.name
    except Exception:  # <-- Which exception can I use to pass without error?
        return "None"
while open_proc is True:
    data = list(psutil.process_iter())
    if any(["gedit" in safe_name(Proc) for Proc in data]):
        open_proc = True
    else:
        open_proc = False
while open_proc:
    data = list(psutil.process_iter())
    open_proc = any(["gedit" in safe_name(proc) for proc in data])

Context

StackExchange Code Review Q#54836, answer score: 3

Revisions (0)

No revisions yet.