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

Command line IRC client

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

Problem

I made this IRC client ages ago in Python, and thought I'd revisit it for Python 3.5 (I'd like to play with asyncio). Before I start, I'd like to have an overall design review about this.

Known issues, although feel free to comment on them:

  • I do too much in one class



  • There is a fair bit of repetition that could probably be pulled out



I don't think there are any obvious bugs, but I never had a rigorous test suite for this (read as: there was never a test suite, I wrote this before I discovered unit testing) so its very possible I've missed something.

I'm also not an expert in the IRC spec - this was mostly googling and trial and error. If I've misunderstood/misused/missed something please comment on that as well.

```
from __future__ import absolute_import, print_function, unicode_literals

from functools import partial
from multiprocessing import dummy
import datetime
import enum
import logging
import select
import socket
import time
import threading

now = datetime.datetime.now()
logging.basicConfig(
filename=''.join(["Logs/", str(datetime.datetime.now()), ".log"]),
level=logging.INFO
)

ErrorCodes = enum(
'ErrorCodes',
'UNKNOWN_HOST UNKNOWN_FAILURE UNKNOWN_CHANNEL UNKOWN_USER '
'MESSAGE_SENT PONG_SUCCESS SERVER_CONNECTED HOSTNAME_NOT_RESOLVED '
'SERVER_DISCONNECTED CHANNEL_JOINED CHANNEL_NAME_MALFORMED CHANNEL_LEFT '
)

class IrcMember(object):
"""Represents an individual who uses the IRC client.

Only stores non-sensitive information.

Attributes
----------
nickname : str
User nickname.
real_name : str
User's "real" name.
ident : str
User's id
servers: dict
Server name to socket mapping for connected servers.
server_channels: dict
Server name to a list of channel names.
server_data: dict
Server name to dict of user information if it differs from the
default values.
lock: threading.Lock
Socket lock.
replies: dict

Solution

In send_server_message you should be using EAFP (Easier to Ask Forgiveness than Permission). You test if hostname not in self.servers, but surely this would normally not be the case? You should instead use a try except, and catch a KeyError on the rare occasion this condition arises:

try:
        sock = self.servers[hostname]
    except KeyError:
        logging.warning("No such server {}".format(hostname))
        logging.warning("Failed to send message {}".format(message))
        return ErrorCodes.UNKNOWN_HOST


Also semantically, I would prefer not to return from inside the else, just have the return outside of a block since there's no other way it would reach that point.

It may be worth having this as a get_host or validate_host function too. You repeatedly use a similar pattern. Confusingly though, it seems like you don't always give the same error back? Having a single function prevents duplicate code and makes errors more clear.

I also take issue with returning MESSAGE_SENT as an apparent error code. Surely that's a successful result? It could be clearer to either have a separate enum for successful results, or to rename it to something like ResultCodes.

Code Snippets

try:
        sock = self.servers[hostname]
    except KeyError:
        logging.warning("No such server {}".format(hostname))
        logging.warning("Failed to send message {}".format(message))
        return ErrorCodes.UNKNOWN_HOST

Context

StackExchange Code Review Q#116793, answer score: 6

Revisions (0)

No revisions yet.