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')