3
\$\begingroup\$

I'm a newbie Python programmer and I just made my first script which allows autoclicking into various positions of the screen (3 for now) an X number of times with an Y interval.

I'd like to know what do you think about my code, specifically if there is a more efficient way to do the same task.

No need to extend functionalities and such. I'm just curious to hear what more experienced developers think.

#import libraries
import pyautogui
from tkinter import *
import time
import subprocess


#settings
pyautogui.PAUSE = 1
pyautogui.FAILSAFE = True
pyautogui.size()
width, height = pyautogui.size()
initialStr = "Screen Size: " + str(width) +" - " +str(height)
print(initialStr)
x,y = pyautogui.position()
positionStr = 'X: ' + str(x).rjust(4) + ' Y: ' + str(y).rjust(4)
print(positionStr)




# displays screen size
def function():
    print(initialStr)

#saves mouse position 1
def position():
    time.sleep(2)
    global xmouse, ymouse
    xmouse, ymouse = pyautogui.position()
    print(str(xmouse)+","+str(ymouse))

    w2 = Label(ro, text="Position 1 set: "+str(xmouse)+","+str(ymouse)).grid(row=2,columnspan=2)

#saves mouse position 2
def position2():
    time.sleep(2)
    global xmouse2, ymouse2
    xmouse2, ymouse2 = pyautogui.position()
    print(str(xmouse2)+","+str(ymouse2))

    w3 = Label(ro, text="Position 2 set: "+str(xmouse2)+","+str(ymouse2)).grid(row=4,columnspan=2)


#saves mouse position 3
def position3():
    time.sleep(2)
    global xmouse3, ymouse3
    xmouse3, ymouse3 = pyautogui.position()
    print(str(xmouse3)+","+str(ymouse3))

    w4 = Label(ro, text="Position 3 set: "+str(xmouse3)+","+str(ymouse3)).grid(row=6,columnspan=2)   

#saves number of cycles
def sel():
    selection = "Value = " + str(iterations.get())
    label = Label(ro, text="Number of cycles: "+str(iterations.get())).grid(row=11,columnspan=2)

#saves execution interval
def sel2():
    selection = "Value = " + str(parametro_timer.get())

    label2 = Label(ro, text="Execution interval set at: "+str(parametro_timer.get())+" seconds").grid(row=14,columnspan=2)


#starts autoclicking
def gogo():

    #checks for unset variables, if one or more are unset it returns an error
    try:
        xmouse,xmouse2,xmouse3
    except NameError:
        label_error = Label(ro, foreground="red", text="ERROR: Some parameters are not set").grid(row=16,columnspan=2)


    #if all settings have been set then the program can start autoclicking
    else:
        time.sleep(2)
        timer=int(parametro_timer.get())
        parametro_range=int(iterations.get())
        for i in range(0,parametro_range):

            pyautogui.click(xmouse, ymouse)
            time.sleep(timer)
            pyautogui.click(xmouse2, ymouse2)
            time.sleep(timer)
            pyautogui.click(xmouse3, ymouse3)
            time.sleep(timer)



#GUI      
ro = Tk()
ro.wm_title("AutoClicker1.0")

#scale variables
iterations = DoubleVar()
parametro_timer = DoubleVar()


w1 = Label(ro, text=initialStr).grid(row=0,columnspan=2, pady=15)    

w2 = Label(ro, text="Position 1 is unset").grid(row=2,columnspan=2)
w3 = Label(ro, text="Position 2 is unset").grid(row=4,columnspan=2)
w4 = Label(ro, text="Position 3 is unset").grid(row=6,columnspan=2)  


button = Button(ro, text="Set Position 1 [2 seconds to hover into position]", command=position).grid(row=1,columnspan=2)
button = Button(ro, text="Set Position 2 [2 seconds to hover into position]", command=position2).grid(row=3,columnspan=2)
button = Button(ro, text="Set Position 3 [2 seconds to hover into position]", command=position3).grid(row=5,columnspan=2)


scale = Scale( ro, variable = iterations, orient=HORIZONTAL, from_=5, to=100 ).grid(row=9,columnspan=2)


button = Button(ro, text="Set number of cycles",  command=sel).grid(row=10,columnspan=2)
label = Label(ro, text="Cycles are unset (Default=5)").grid(row=11,columnspan=2)


scale1 = Scale( ro, variable = parametro_timer, orient=HORIZONTAL, from_=2, to=15 ).grid(row=12,columnspan=2)
button = Button(ro, text="Set execution interval",  command=sel2).grid(row=13,columnspan=2)
label2 = Label(ro, text="Execution interval is unset (Default=2)").grid(row=14,columnspan=2)


button = Button(ro, text="Start", command=gogo).grid(row=15,columnspan=2,padx=110,pady=5)

label_error = Label(ro, text="").grid(row=16,columnspan=2)

ro.mainloop()
\$\endgroup\$
2
\$\begingroup\$

Review

PEP 8

There are rules in Python, how you should format your code. If you want to write proper code, you should follow it: https://www.python.org/dev/peps/pep-0008/

Use an IDE

I suggest using a good IDE for Python, since it will give you hints about errors in your code. As I opened your code, my IDE found 6 warnings and 79 weak warnings. Here is a list of IDEs: https://realpython.com/python-ides-code-editors-guide/

Remove unused code

If there is any unused code, please remove it. See line 22 to 24:

# displays screen size
def function():
    print(initialStr)

Organize Imports

Use f-Strings

In line 12 to 13 you built a String this way:

width, height = pyautogui.size()
initialStr = "Screen Size: " + str(width) +" - " +str(height)

Instead, you could use f-Strings, which make it way easier:

width, height = pyautogui.size()
initialStr = f"Screen Size: {width} - {height}"

More information: https://realpython.com/python-f-strings/

Use IntVar for iterations

Since all possible values for iterations are integers, you should use an IntVar instead of DoulbeVar here (line 97, iterations = DoubleVar()). Your Iterations Scale will still work fine.

Use if name == "main"

Wrap your main code with if __name__ == "__main__":. See here why: https://stackoverflow.com/questions/419163/what-does-if-name-main-do

Use constants

It's nice to declare constants, especially inside classes, so they are not in global scope. This way, you can modify simple values easily, no need for searching them inside your code. See example.

Merge similar functions in one

There is a programmer inside joke that says "no self respecting ethical programmer would ever consent to write a bomb Baghdad function. They would write a bomb city function to which Baghdad could be passed as a parameter." (source: https://codereview.stackexchange.com/a/204210/179357).

From line 26 to 52 you wrote the following code:

#saves mouse position 1
def position():
    time.sleep(2)
    global xmouse, ymouse
    xmouse, ymouse = pyautogui.position()
    print(str(xmouse)+","+str(ymouse))

    w2 = Label(ro, text="Position 1 set: "+str(xmouse)+","+str(ymouse)).grid(row=2,columnspan=2)

#saves mouse position 2
def position2():
    time.sleep(2)
    global xmouse2, ymouse2
    xmouse2, ymouse2 = pyautogui.position()
    print(str(xmouse2)+","+str(ymouse2))

    w3 = Label(ro, text="Position 2 set: "+str(xmouse2)+","+str(ymouse2)).grid(row=4,columnspan=2)


#saves mouse position 3
def position3():
    time.sleep(2)
    global xmouse3, ymouse3
    xmouse3, ymouse3 = pyautogui.position()
    print(str(xmouse3)+","+str(ymouse3))

    w4 = Label(ro, text="Position 3 set: "+str(xmouse3)+","+str(ymouse3)).grid(row=6,columnspan=2)

As you can see, you wrote nearly the same code three times. Whenever you see this, you should write one function and pass a parameter to determine, which position you address. I fixed this in my example below.

Avoid global

Using globals like you did in line 29 global xmouse, ymouse is bad programming style. See here why: https://stackoverflow.com/questions/423379/using-global-variables-in-a-function

Shadowing scope variables

It seems like you have been trying to declare variables and to modify them iside your functions. For example: You declare label in line 117:

label = Label(ro, text="Cycles are unset (Default=5)").grid(row=11,columnspan=2)

In function sel you try to override the label (line 57):

label = Label(ro, text="Number of cycles: "+str(iterations.get())).grid(row=11,columnspan=2)

But this way you cannot address label from outer scope, instead you define a new variable, which is useless. Now your workaround by using global might come into mind. There are two solutions to this problem:

  1. Pass the variable as a parameter. E.g. def sel(label):
  2. Use classes, define members and reference them inside your methods. I suggest this way, as I did in my example below.

Reuse objects

Now since we understand how to reference variables, we might manipulate them instead of creating new ones. So instead of creating new Labels and placing them over other labels (you can see that in your GUI, yikes!), you can simply modify them by using label.config(text="foo").

Don't block your GUI

time.sleep(int) blocks your whole GUI. It may work for your 2-second freeze to find mouse positions, but it's not nice for executing your autoklick event. Instead, you should start new threads. Simply look at my example, you can find an implementation there.

Add a way to cancel autoclicking

It's a very important feature for your Autoclicker to be able to interrupt the script. See example


Example

autoclicker.py

import tkinter as tk
import pyautogui
import time
import threading


# inherits from Tk class, so each autoclicker is a Tk itself
class Autoclicker(tk.Tk):

    POSITION_COUNT_DEFAULT = 3

    TITLE = "AutoClicker1.0"

    ITERATIONS_MIN = 5
    ITERATIONS_MAX = 100
    ITERATIONS_RESOLUTION = 1

    TIMER_MIN = .2
    TIMER_MAX = 5
    TIMER_RESOLUTION = .2

    DELAY = 2  # in s

    # creates all widgets, variables...
    def __init__(self, pos_count=POSITION_COUNT_DEFAULT):

        tk.Tk.__init__(self)

        self.title(self.TITLE)

        self.pos_count = pos_count
        self.positions = [None] * pos_count

        # variables for scales and checkbuttons
        self.iterations = tk.IntVar()
        self.timer = tk.DoubleVar()
        self.doubleclick = tk.BooleanVar()

        width, height = pyautogui.size()
        self.label_dim = tk.Label(self, text=f"Screen Size: {width} x {height}")

        self.labels_position = []
        self.buttons_position = []

        for i in range(pos_count):
            self.labels_position.append(tk.Label(self, text=f"Position {i + 1} is unset"))
            self.buttons_position.append(tk.Button(self, text=f"Set Position {i + 1}",
                                                   command=lambda i_cpy=i: self.position(i_cpy)))

        self.scale_iterartions = tk.Scale(
            self,
            variable=self.iterations,
            orient=tk.HORIZONTAL,
            from_=self.ITERATIONS_MIN,
            to=self.ITERATIONS_MAX,
            resolution=self.ITERATIONS_RESOLUTION,
            label="Iterations:"
        )
        self.scale_timer = tk.Scale(
            self,
            variable=self.timer,
            orient=tk.HORIZONTAL,
            from_=self.TIMER_MIN,
            to=self.TIMER_MAX,
            resolution=self.TIMER_RESOLUTION,
            label="Timer:"
        )
        self.checkbutton_doubleclick = tk.Checkbutton(self, variable=self.doubleclick, text="Doubleclick")
        self.button_start = tk.Button(self, text="Start", command=self.start)
        self.label_error = tk.Label(self, foreground="red")

        # layout
        self.label_dim.pack()
        for i in range(pos_count):
            self.labels_position[i].pack()
            self.buttons_position[i].pack()
        self.scale_iterartions.pack()
        self.scale_timer.pack()
        self.checkbutton_doubleclick.pack()
        self.button_start.pack()
        self.label_error.pack()

        # set keyboard events
        self.bind("<Key-Escape>", self.cancel)

        # init cancel var, needed to interrupt autoclicking
        self.cancel = False

    # saves mouse positions
    def position(self, i):
        self.set_busy(True)  # lock ui

        def callback():
            time.sleep(self.DELAY)
            self.positions[i] = pyautogui.position()
            xmouse, ymouse = self.positions[i]
            self.labels_position[i].config(text=f"Position {i + 1} set: {xmouse}, {ymouse}")
            self.set_busy(False)  # unlock ui

        threading.Thread(target=callback).start()

    # locks/unlocks all widgets
    def set_busy(self, busy):
        state_new = tk.DISABLED if busy else tk.NORMAL 
        for button in self.buttons_position:
            button.config(state=state_new)
        self.scale_iterartions.config(state=state_new)
        self.scale_timer.config(state=state_new)
        self.checkbutton_doubleclick.config(state=state_new)
        self.button_start.config(state=state_new)

    # starts autoclicking
    def start(self):

        def callback():
            for _ in range(self.iterations.get()):
                for position in range(self.pos_count):
                    if not self.cancel:
                        if self.doubleclick.get():
                            pyautogui.doubleClick(self.positions[position])
                        else:
                            pyautogui.click(self.positions[position])
                        time.sleep(self.timer.get())
            self.cancel = False
            self.set_busy(False)  # unlock ui

        # checks for unset variables, if one or more are unset it returns an error
        if None in self.positions:
            self.label_error.config(text="ERROR: Some positions are not set")

        # if all settings have been set then the program can start autoclicking
        else:
            self.set_busy(True)  # lock ui
            self.cancel = False
            self.label_error.config(text="")
            threading.Thread(target=callback).start()

    # interrupts autoclicking (press Escape-Key)
    def cancel(self, _):
        self.cancel = True


if __name__ == "__main__":
    # change the parameter to any number of positions you want to have
    Autoclicker(2).mainloop()
\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for spending your time improving my code. There's much I don't understand yet but I'll keep studying it. I'll make sure to vote once I have enough reputation. \$\endgroup\$ – John Wilver Nov 16 '18 at 9:43
  • \$\begingroup\$ You can still accept this answer if it solves your problem. If not, you don't have to. \$\endgroup\$ – saucecode Nov 16 '18 at 13:04
  • \$\begingroup\$ Right, I've noticed that the escape key doesn't work to stop the script though, is it an OS related problem? \$\endgroup\$ – John Wilver Nov 16 '18 at 13:42
  • \$\begingroup\$ I found out why. The bind() method only works within the mainloop. So I changed it with FocusIn, that way the script will halt once I re open the program's gui. \$\endgroup\$ – John Wilver Nov 16 '18 at 14:19
  • \$\begingroup\$ U may also try to execute self.focus_set() in Autoclicker.init \$\endgroup\$ – saucecode Nov 16 '18 at 14:31

Your Answer

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

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