HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonModerate

Replacing Skype, reinventing the chat client

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thechatskypereinventingclientreplacing

Problem

I'm creating a chat client and chat server after a several month programming hiatus. The goal is to make one with features I want but it's also a learning project. Although it's mostly because I'm annoyed at cough skype.

I've realised I don't know if anything I've written currently is any good (never had anyone critique my code) so I'd like to know if the structure of the program is any good (especially how I handle tkinter and classes) and my use of control flow etc.

The code currently has 2 classes, one that contains the GUI elements (tkinter) and the other handles the networking side (threading and socket). It creates a thread for the listen method so it is constantly awaiting data from the server. Whilst the initial thread/process is used for the tkinter mainloop.

You can get/see the tkHyperlinkManager file here and the server here

```
try: #if python3
from tkinter import *
import tkinter.font as tkFont
except: #if python2
from Tkinter import *
import tkFont

from socket import *
from threading import *
import time,sys, json,tkHyperlinkManager, webbrowser #tkHyperlinkManager is another file, I didn't write it.

class chatGUI: #Class That Handles GUI related tasks.
def __init__(self):#Initializing window settings
self.bgColour = "#2a2a2a" #First COlour was: "#607D8B"
window.title("Oke's Chat Client")
window.configure(bg=self.bgColour)
self.chat = Frame(window,bg=self.bgColour)
self.menu = Frame(window,bg=self.bgColour)
self.checkFile() #Check for options/settings configuration
menuBar = Menu(window,foreground=self.bgColour,activeforeground=self.bgColour)
menuBar.add_command(label="Options", command=self.optionMenu)
window.config(menu=menuBar)
self.chatFrame()#Loads Chat GUI into memory.
#Load the first Menu
self.menuFrame()#Loads the MENU GUI into memory.
self.menu.place(y=0,x=0,height=500,width=500)
self.hyperList = ["http://",

Solution

In your chatGUI's __init__ your setting 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."""

Code Snippets

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)
class chatGUI: 
    """Handles GUI related tasks."""
if not self.alias.isspace() and
if not self.alias.isspace() and 0 < len(self.alias) < 16:
except Exception as error:
    print("Unable to connect to server")
    print(error.message)

Context

StackExchange Code Review Q#120847, answer score: 16

Revisions (0)

No revisions yet.