Try/except code style sugggestions

https://github.com/corrad1nho/qomui/blob/fe981c12f0bbc73f0b953883b00763703cf2b01c/qomui/qomui_gui.py#L1230

Hi! I noticed that you said this was a project for you to work on your python, so I hope you don't take offense to what I mean to be constructive feedback. I was looking through some of the code and noticed that you're kind of abusing try/except statements. They're not really meant to be used when the success of the process can be known beforehand e.g. checking if a key exist in a dictionary. They're meant to be used when a process is occurring that may or may not fail, and if it does it should not break your program e.g. opening a file.

I did a brief rewrite of the referenced function to demonstrate a way I think is a cleaner and more pythonic way to do the same logic (this may not be a perfect implementation of what you had previously, I'm trying to portray the ideas). There are also many cases in the function where if the condition is not met, the function just falls through. This is generally a bad practice because if unexpected values are present the program will fail silently, at the very least the discrepancy should be logged. I have the function raise errors where you'd probably prefer to log the activity or display a message to the user, but it's important that it no longer silently falls through.

def setting_enabled(dict, key):
    """Checks if a setting exists in the given dictionary and if it is enabled
    """
    if key in dict and (dict[key] == 1 or self.dict[key] == 'on'):
        return True
    else:
        return False # May want to return the dafault value of the key here instead

def connect_last_server(self):
    if setting_enabled(self.config_dict, "autoconnect"):
        self.kill()
        self.disconnect_bypass()
        last_server_dict = self.load_json("{}/last_server.json".format(HOMEDIR))

        if self.network_state == 1:

            if "last" in last_server_dict:
                self.ovpn_dict = last_server_dict["last"] 

                if "hop" in last_server_dict and last_server_dict["hop"] is not None:
                    self.hop_server_dict = last_server_dict["hop"]
                    self.show_hop_widget()

                if setting_enabled(self.ovpn_dict, "random"):
                    self.choose_random_server()
                else:
                    if "profile" in self.ovpn_dict:
                        self.connect_profile(self.ovpn_dict["profile"])
                    else:
                        self.establish_connection(self.ovpn_dict)

                if setting_enabled(self.ovpn_dict, "favourite")
                    self.favouriteButton.setChecked(True)

            if "bypass" in last_server_dict:
                self.bypass_ovpn_dict = last_server_dict["bypass"]
                self.establish_connection(self.bypass_ovpn_dict, bar="_bypass")

            # What if both of the above are false?
            # Was a connection established? Or should an error be displayed?

        else:
            raise ValueError('Attempted to connect to last server when network_state == 0')
    else:
        # Perhaps this should just be logged, but if the entire function is dependent on this first line of logic,
        # that logic should probably occur in the calling statement not here
        raise ValueError('Attempted to connect to last server when autoconnect is disabled')

corrad1nho / qomui

Try/except code style sugggestions #72