Python Forum
my code is messy .. how do i not get messy
Thread Rating:
  • 0 Vote(s) - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
my code is messy .. how do i not get messy
#1
I've heard about modular programming where we break the code into a separate module but I don't understand which parts I have to separate and where they all put together... and this my code

I'm here using pysimplegui to make the GUI I think I can make it separate from the functional application .. but I don't understand how to unify them all ...

import time
import requests
import instaloader
import PySimpleGUI as sg
from instaloader import Profile, Post


class InstaDownloader:
    def __init__(self, username, password):
        self.username = username
        self.password = password
        self.root = instaloader.Instaloader(dirname_pattern=self.username,
                                            filename_pattern="InstaFolder")
        self.profile = Profile.from_username(self.root.context, self.username)

    def get_photo_profile(self):
        self.root.download_profilepic(self.profile)

    def get_picture(self, url, timestamp):
        self.root.download_pic(self.root, url, timestamp)

    def get_stories(self):
        self.root.download_profiles([self.profile], profile_pic=False,
                                    posts=False, tagged=False, igtv=False,
                                    highlights=False, stories=True,
                                    fast_update=True, post_filter=None,
                                    storyitem_filter=None, raise_errors=False)

    def get_post(self, shortcode):
        post = Post.from_shortcode(self.root.context, shortcode)
        self.root.download_post(post, self.root)

    def get_igtv(self):
        self.root.download_igtv(self.profile, fast_update=True,
                                post_filter=None)

    def login_user(self):
        self.root.login(self.username, self.password)

    @staticmethod
    def check_connection(url='https://instagram.com/', timeout=3):
        try:
            requests.head(url, timeout=timeout)
            return True
        except requests.ConnectionError:
            return False


def progress_bar(times):
    progress = [[sg.ProgressBar(times, orientation='h',
                                size=(20, 20), key='progressbar')],
                [sg.Cancel()]]

    window = sg.Window('Custom Progress Meter', progress)

    progress_bar = window.FindElement('progressbar')
    for i in range(int(times)):
        event, values = window.Read(timeout=0)

        if event == 'Cancel' or event is None:
            break

        progress_bar.UpdateBar(i + 1)

    window.Close()


login = [[sg.InputText("Username", key="-USERNAME-")],
         [sg.InputText("Password", password_char="*", key="-PASSWORD-")],
         [sg.Button("Submit")]]

window = sg.Window("InstaDownloader", login, size=(200, 100))

if InstaDownloader.check_connection():
    win2_active = False
    while True:
        event, values = window.Read()
        if event in (sg.WIN_CLOSED, "Exit"):
            break
        if event == "Submit" and not win2_active:
            looping = True
            while looping:
                # check user
                objt = InstaDownloader(values["-USERNAME-"],
                                       values["-PASSWORD-"])
                try:
                    objt.login_user()
                    sg.popup("Succes!")
                    looping = False
                except:
                    sg.popup("Incorrect username and password")
            win2_active = True
            window.Hide()
            # create layout for window 2
            Downloader = [[sg.Button("Photo Profile"),
                           sg.Button("Picture"),
                           sg.Button("Stories")],
                          [sg.Button("Post"),
                           sg.Button("IGTV")]]

            window2 = sg.Window("InstaDownloader", Downloader, size=(500, 230))
            pic_active = False
            post_active = False
            # start window 2
            while True:
                event2, values2 = window2.Read()
                if event2 is None or event2 == "Exit":
                    window2.Close()
                    win2_active = False
                    window.UnHide()
                    break
                if not pic_active and event2 == "Picture":
                    pic_active = True
                    pic_gui = [[sg.InputText("URL", key="-URL-")],
                               [sg.InputText("TimeStamp", key="-TIMESTAMP-")],
                               [sg.Button("Submit")]]

                    pic_win = sg.Window("InstaDownloader", pic_gui,
                                        size=(200, 100))
                if pic_active:
                    event4, values4 = pic_win.Read()
                    if event4 is None or event4 == "Exit":
                        pic_active = False
                        pic_win.Close()
                    # must get URL and Timestamp to download
                    objt.get_picture(values4["-URL-"], values["-TIMESTAMP-"])
                if not post_active and event2 == "Post":
                    post_active = True
                    post_gui = [[sg.InputText("Shortcode", key="-SHORTCODE-")],
                                [sg.Button("Submit")]]

                    post_win = sg.Window("InstaDownloader", post_gui,
                                         size=(200, 100))
                if post_active:
                    event5, values5 = post_win.Read()
                    if event5 is None or event5 == "Exit":
                        post_active = False
                        post_win.Close()
                    # have to get the shortcode to download
                    objt.get_post(values5["-SHORTCODE-"])
                if event2 == "Photo Profile":
                    # How can I not declare a variable multiple times?
                    start_time = time.time()
                    objt.get_photo_profile()
                    progress_bar(time.time() - start_time)
                if event2 == "Stories":
                    # How can I not declare a variable multiple times?
                    start_time = time.time()
                    objt.get_stories()
                    progress_bar(time.time() - start_time)
                if event2 == "IGTV":
                    # How can I not declare a variable multiple times?
                    start_time = time.time()
                    objt.get_igtv()
                    progress_bar(time.time() - start_time)
else:
    sg.popup("Lost Connection!")
I'm too used to making applications in one module and lately I'm starting to worry about it...
Reply
#2
1) yes your code is messy. Yes it can be improved.
2) but first, answer this: why?

If you're only making this for your own use, I'm not sure it really matters. If your goal is to share it with others, then it makes sense to clean it up a bit.

As to how to improve it, think about separation of concerns, and having a function do only one thing. The progress_bar() function, for example, does a lot more than just display a progress bar. It's creating a new window, running an event loop (window.Read), and then finally creating a progress bar and updating it.

As for the end of the script, where you have "How can I not declare a variable multiple times?", you could do something like this:
actions = {
    "Photo Profile": lambda insta: insta.get_photo_profile(),
    "Stories": lambda insta: insta.get_stories(),
    "IGTV": lambda insta: insta.get_igtv()
}

while True:
    event2, values2 = window2.Read()
    # a few lines skipped
    if event2 in actions:
        start_time = time.time()
        actions[event2](objt)
        progress_bar(time.time() - start_time)
Reply
#3
(Aug-27-2020, 03:44 AM)nilamo Wrote: 1) yes your code is messy. Yes it can be improved.
2) but first, answer this: why?

If you're only making this for your own use, I'm not sure it really matters. If your goal is to share it with others, then it makes sense to clean it up a bit.

As to how to improve it, think about separation of concerns, and having a function do only one thing. The progress_bar() function, for example, does a lot more than just display a progress bar. It's creating a new window, running an event loop (window.Read), and then finally creating a progress bar and updating it.

As for the end of the script, where you have "How can I not declare a variable multiple times?", you could do something like this:
actions = {
    "Photo Profile": lambda insta: insta.get_photo_profile(),
    "Stories": lambda insta: insta.get_stories(),
    "IGTV": lambda insta: insta.get_igtv()
}

while True:
    event2, values2 = window2.Read()
    # a few lines skipped
    if event2 in actions:
        start_time = time.time()
        actions[event2](objt)
        progress_bar(time.time() - start_time)


yeah I want to share it on github and I think it's quite embarrassing if I share it with messy code .. but should I make my code into modules? or only one module and contains a collection of functions? and if it becomes several modules is there a requirement they can be divided into several modules?
Reply
#4
Some thoughts, without pretense to be exhaustive, in addition to what @nilamo said:
  • There is absolutely no need to create your own class. The only thing it does is to wrap instaloader.Instaloader. This is confusing and unnecessary complicating the code. The only different method that does not make use of self.root is check_connection which is @staticmethod because it really has nothing to do with the class anyway.
  • you import instaloader and on separate line import from instaloader Profile and Post. Later on the only place where you use instaloader is to reference instaloader.Instaloader in the __init__ method of InstaDownloader class. Be consistent. My preference would be to always full reference, i.e. drop the from instaloader import Post, Profile and refernce e.g. instaloader.Post in your code.
  • As already mentioned, incl. by you - separate gui and the logic
  • There are almost no comments and no docstrings at all. If you are going to publish it for use from other people they will need documentation. And it's always better for you when maintain the code. This is related to my first bullet - why wrap the instaloader.Instaloader?
  • lines 141-155 are repeating code, i.e. 3 times the same code with only the event2 being different - so you can have a function (or a method if in class). Same looks true for other parts of the code too.
  • names like event, event 2, ... event 4, event5 tell me there is something wrong in what you do or the design, but I didn't look further
I didn't look much into rest of the code
If you can't explain it to a six year old, you don't understand it yourself, Albert Einstein
How to Ask Questions The Smart Way: link and another link
Create MCV example
Debug small programs

Reply


Forum Jump:

User Panel Messages

Announcements
Announcement #1 8/1/2020
Announcement #2 8/2/2020
Announcement #3 8/6/2020