Minimal, Low-level Telnet API

I've started Python some days ago and wrote a small Telnet implementation according to RFC 854 and RFC 5198. It is pretty low-level and sending messages directly isn't even supported. The highest abstractions are commands and user data where commands include negotiation (like "Do") and "general-purpose" commands (like "Go Ahead").

The best thing about it is that, using the __bytes__ method, these data can be converted to their binary representation and directly embedded into a data stream using TCP or just storing it temporarily, making it very flexible.

Code (libtelnet.py):

```
# This module contains code and data to provide a slight abstraction from
# the socket module to provide Telnet client facilities.

import enum
import socket
import struct

# The Telnet protocol is based on communication between two NVTs (Network
# Virtual Terminals). They are an abstraction of terminals, so the protocol
# can operate independently of the actual terminal used.

# Standard Telnet option negotation syntax (partially quoted from
# RFC 854):
# - WILL XXX: indicate local party's desire (an offer) to begin performing
# option XXX
# - DO/DON'T XXX: positive/negative acknowledgements of WILL XXX
# - DO XXX: tell other party to begin performing option XXX
# - WILL/WON'T XXX: positive/negative acknowledgements of DO XXX

# An NVT consists of a printer and a keyboard; the former for outputting
# information, the ladder for inputting it. By default, the NVT is a half-duplex
# device in line-buffered mode. This can be altered by options, though.

# The Go Ahead (GA) command is to be issued when the Telnet client has sent all
# data to the printed and no input from the keyboard is waiting to support
# "lockable" keyboards at a half-duplex connection, for instance, the IBM 2741:
# it is locked when the other end is sending. The GA command unlocks it again.
# The GA command's purpose is not to signalize an EOL; usually, the machine
# has other mechanisms for handling and

Q&A

1) This is wrong. Actually from xxx import * is considered bad practice, for
the exact reason you point out. It is only acceptable for very specific modules
like pylab, which have a specific intent doing so (setup a MATLAB-like
environment in this case).

2) Enums are very new in Python, so it is easy to argue that you don't need them
at all. But I feel your use is consistent, even if it seems quite proeminent
at the moment in front of the actual do-something part of your code.

String Manipulation, Generator Expression

I focus on the _repr function. Avoid concatenating strings with the +
operator. This is because strings are immutable, so you actually create a copy
of concatenated strings "each time" (okay, sometimes Python is more intelligent
than that but this is off-topic for a start). Use format and join
whenever you can. You will improve readability.

You know what list comprehensions do. Well done. But remember that they
build a list, eating up memory. For all functions that accept an iterable,
you can replace this by a generator expression (just remove the []).

def _repr(self):
    return "".format(
        members=", ".join(
            "{key:!s}={value:!s}".format(key=key, value=value)
            for key, value in self.__dict__.items()
        )
    )

Same remark for UserData.__init__:


for + str + "+=" = FAIL

Place all string chunks in a list, join them at the end.

class UserData:
    def __init__(self, data):
        data_elements = []

        for i in data:
            if isinstance(i, CtrlChars):
                data_elements.append(i.value)
            else:
                data_elements.append(i)

        self._data_str = "".join(data_elements)

Your note on inefficiency

You gain a lot by waiting for the very last moment to process data. At least in
performance: if you don't need, you don't do. You avoid unnecessary actions.
But also in readability. If you do things when you need them, it's more clear
why you do it, and also what you are doing.


Now is better than never.

Although never is often better than right now.

— The Zen of Python, Tim Peters.

One idea is to setup an invalidation mechanism. This delays the creation of
the bytes to when it is really needed. Below is only an example.

class Command:
    def __init__(...):
        super().__setattr__("_bytes_valid", False)
        super().__setattr__("_bytes", b'')
        # ...

    def __bytes__(self):
        # Recalc on access if necessary (cache-like)
        if not self._bytes_valid:
            super().__setattr__("_bytes", self._calc_bytes())
            super().__setattr__("_bytes_valid", True)
        return self._bytes

    def __setattr__(self, name, value):
        super().__setattr__(name, value)
        super().__setattr__("_bytes_valid", False)

We should forget about small efficiencies, say about 97% of the time:
premature optimization is the root of all evil. Yet we should not pass up our
opportunities in that critical 3%.

— Structured Programming with Goto Statements, Donald Knut.

The question being are we really in that critical 3%? Considering how often
the __bytes__ method is supposed to be called (probably once before sending
the command), and how many times an attribute is set (at least 3 times with
cmd, option and option_args), IMHO the best solution in that case is

class Command:
    def __bytes__(self):
        return self._calc_bytes()

Tips & Tricks

class Command:
    def __repr__(self):
        return _repr(self)

can be replaced by

class Command:
    __repr__ = _repr

This will save you a function call.

The last parameter of Command.__init__ is a good candidate for argument unpacking.

def __init__(self, cmd, option=None, *option_args):

Blocks cannot be empty. That's why you have to use the pass statement.

class SuboptHandlers:
    """Contains handler functions, which are called when subnegotation
       should take place. They return a bytes object containing an adequate
       response to the subnegotation from the Telnet server.
    """
    pass # There

Final thoughts on design

Of course, this is MY review, not THE review. My opinion can be discussed.

(Beware, big blahblah ahead).

It is bad practice to code without having layed out a solid design. Roughly
speaking, you collect use cases and requirements, select an architecture and
refine your design. Only then you write code.

Extensibility shall be chosen by design and not by failing a design.
Because it comes at the price of performance, readability and simplicity, it
has to be a required feature of your design. "if I want to add something" is
not a good reason. "when I will add that feature" is.

That's why, considering the actual state of your code, I suggest you use
functions rather than classes and standard containers rather than objects.


Simple is better than complex. [...]

Flat is better than

StackExchange Code Review Q#119314, answer score: 2