Skip to main content
edited body
Source Link
SuperBiasedMan
  • 13.1k
  • 5
  • 36
  • 62

In your chatGUI's __init__ your settinysetting up some constants, that should really just be defined as constants at the class level:

In your chatGUI's __init__ your settiny up some constants, that should really just be defined as constants at the class level:

In your chatGUI's __init__ your setting up some constants, that should really just be defined as constants at the class level:

Source Link
SuperBiasedMan
  • 13.1k
  • 5
  • 36
  • 62

In your chatGUI's __init__ your settiny up some constants, that should really just be defined as constants at the class level:

class chatGUI: #Class That Handles GUI related tasks.
    BGCOLOUR = "#2a2a2a"  # First Colour was: "#607D8B"
    # Contains hyperlink triggers.
    HYPER_LIST = ("http://", "www.", "https://", "ftp://")
    WINDOW_TITLE = "Oke's Chat Client"
    
    def __init__(self):#Initializing window settings
        window.title(self.WINDOW_TITLE)
        window.configure(bg=self.BGCOLOUR)
        self.chat = Frame(window, bg=self.BGCOLOUR)
        self.menu = Frame(window, bg=self.BGCOLOUR)
        

This makes it clearer that these are constants that are not dependent on how an instance is initialised, they are always the same. There are others that could definitely go here too, not just things from __init__. The path to your bgImg for instance, the size and position for your menu. You should use constants for most values that are fixed and predefined. Having them at the top of the class or top of the script makes it easy for people to refer back to what they all are, and allows for easier tweaking if you need to adjust them later.

Now you have some commenting issues. All of the comments in the code above could probably be edited or removed. First, use docstrings when you want to explain what a class or function is:

class chatGUI: 
    """Handles GUI related tasks."""

This is the convention, and it's accessible programmatically so it's more helpful than a comment. Next, does a user need to know what the "First Colour" was? If there's a reason it's not entirely clear. You don't need to comment about something unless it's actually relevant and necessary.

Instead of commenting # Contains hyperlink triggers rename your list HYPERLINK_TRIGGERS so that it's always clear what it contains. Lastly people generally know what __init__ is so there's not much need to comment what that does. Only note if something is unusual. Though in this case I might argue that you should explain where window comes from. You use it heavily in this initialisation and it confused me at first. It may just be a tkinter thing (I'm not familiar with tkinter) but if this isn't common you should explain it, or better yet pass window to __init__ to make it explicitly connected.

In switchToChat you use some unusual logic in your if call. Instead of using == False it's best to use not.

    if not self.alias.isspace() and 
    

You can test self.alias != "" just using Python's truthiness, where if self.alias will evaluate as True for any non empty string, which is exactly what you need. But since you're using len anyway, it would be easier to just compare there:

    if not self.alias.isspace() and 0 < len(self.alias) < 16:

Now you're ensuring len is between 0 and 16 characters. One note, you're not accounting for trailing whitespace. If I enter "SuperBiasedMan " then I'll fail the check, but commonly people strip out the whitespace from inputs. I'd recommend calling .strip() on alias, which also means you could remove the isspace() test and just check the length. It's also odd that you're not doing anything for an invalid alias. You should at least print an explanation, ideally saying what parameters are valid so they can re-enter them.

You should not use a bare except to swallow exceptions. That means literally any error will be ignored. If there are many unpredictable possible errors, at least print the error so the user can read what happened

except Exception as error:
    print("Unable to connect to server")
    print(error.message)

In checkFile you should use with when opening a file. with ensures that the file is closed regardless of what exceptions may occur. It's the safest way to open and close your file. You also use json.loads(optionFile.read()) but you can juse use json.load(optionFile) and then the json module can read the file itself.

    try:
        with open("options.txt") as optionFile:
            self.optionData = json.load(optionFile)

You can do the same with json.dump to not have to write the file directly:

        with open("options.txt", "w") as optionFile:
            self.optionData = {
                            "timeStamp": 1,
                            "timeSet": 1
                         }
            json.dump(self.optionData, optionFile)

Note I removed the truncating, changed to "w" mode and skipped the file reading because they seemed unnecessary. You don't need to read the information you've just written, you still have the dictionary. There's also no need to call truncate, the whole file is wiped when you open it with write mode anyway. And 'w+' mode is just when you want to open a file to read and to write, which you don't need here. You can similarly trim this in saveSettings

def saveSettings(self):
    """Save setting vars to txt file. Called by Apply button push."""
    
    with open("options.txt", "w") as optionFile:
        json.dump(self.optionData, optionFile)

In displayData you have two long very similar lines with small differences. It would be more readable to only adjust the small difference in the if else blocks and then keep the full line after:

    if self.optionData["timeStamp"]:
        if self.optionData["timeSet"]:  # Using 24HR
            time_format = '%H:%M'
        else:
            time_format = '%I:%M'
        self.mainText.insert(END, time.strftime(time_format, time.localtime()) + " - "+str(data[0]), 'bold')