Return to Answer

Direct access to dict is OK, reworked the section on inefficiency and explain better classes vs funcs

edited Feb 9, 2016 at 16:51

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

You can usegain 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)
 selfsuper()._bytes_valid =__setattr__("_bytes", Falseb'')
 self._bytes =# b''...
 
 def __bytes__(self):
 # Recalc on access if necessary (cache-like)
 if not self._bytes_valid:
 selfsuper()._bytes =__setattr__("_bytes", self._calc_bytes())
 selfsuper()._bytes_valid =__setattr__("_bytes_valid", True)
 return self._bytes
 def __setattr__(self, name, value):
 super().__setattr__(name, value)
 selfsuper()._bytes_valid =__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()

self.__dict__["cmd"] = cmd
self.__dict__["option"] = option

No.

self.cmd = cmd
self.option = option

Yes.

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

Final thoughts on complexitydesign

You gain a lot by waiting for the very last momentOf course, this is MY review, not THE review. My opinion can be discussed.

(Beware, big blahblah ahead).

It is bad practice to process datacode without having layed out a solid design. At least inRoughly performance: if you don't needspeaking, you don't do. You avoid unnecessary actions.collect use cases and requirements, select an architecture and But also in readabilityrefine your design. If you do things whenOnly then you need them, it's more clearwrite code.

Extensibility shall be chosen by design and not by failing a design. why you doBecause it comes at the price of performance, readability and also what you are doingsimplicity, 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.

At its current stateThat's why, considering the actual state of your code is too big to do that few. There is no point in, I suggest you use usingfunctions rather than classes to do the joband standard containers rather than objects.

Simple is better than complex. [...]
Flat is better than nested. — The Zen of Python , Tim Peters.

def userdata_as_bytes(data):
 data_elements = []
 for i in data:
 if isinstance(i, CtrlChars):
 data_elements.append(i.value)
 else:
 data_elements.append(i)
 return ("".join(data_elements)).encode("ascii")
def command_as_bytes(command):
 # In this implementation example, command is just a listtuple or tuplea list
  # containing (cmd, option, ...). This is not required, but shows you
 # how standard containers can also be suited for encapsulation.
 # You could instead declare
 # command_as_bytes(cmd, option=None, *option_args)
 cmd, option, *option_args = command
 
 # Validation
 if (not isinstance(cmd, Commands)):
 raise TypeError("invalid Telnet command")
 if (not isinstance(option, Options)):
 raise TypeError("invalid Telnet option")
 
 data = [IAC, cmd.value]
 
 if option is not None:
 data.append(option.value)
 data += [optarg.value for optarg in option_args]
 
 return bytes(data)

Be lazy. Delay actions till you are sure you (will) need them.
— A stupid quote, Me

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

You can use an invalidation mechanism.

class Command:
 def __init__(...):
 # ...
 self._bytes_valid = False
 self._bytes = b''
 
 def __bytes__(self):
 # Recalc on access if necessary (cache-like)
 if not self._bytes_valid:
 self._bytes = self._calc_bytes()
 self._bytes_valid = True
 return self._bytes
 def __setattr__(self, name, value):
 super().__setattr__(name, value)
 self._bytes_valid = False

self.__dict__["cmd"] = cmd
self.__dict__["option"] = option

No.

self.cmd = cmd
self.option = option

Yes.

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

Final thoughts on complexity

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.

At its current state, your code is too big to do that few. There is no point in using classes to do the job.

def userdata_as_bytes(data):
 data_elements = []
 for i in data:
 if isinstance(i, CtrlChars):
 data_elements.append(i.value)
 else:
 data_elements.append(i)
 return ("".join(data_elements)).encode("ascii")
def command_as_bytes(command):
 # command is just a list or tuple (cmd, option, ...)
 cmd, option, *option_args = command
 
 # Validation
 if (not isinstance(cmd, Commands)):
 raise TypeError("invalid Telnet command")
 if (not isinstance(option, Options)):
 raise TypeError("invalid Telnet option")
 
 data = [IAC, cmd.value]
 
 if option is not None:
 data.append(option.value)
 data += [optarg.value for optarg in option_args]
 
 return bytes(data)

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.

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

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

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 nested. — The Zen of Python , Tim Peters.

def userdata_as_bytes(data):
 data_elements = []
 for i in data:
 if isinstance(i, CtrlChars):
 data_elements.append(i.value)
 else:
 data_elements.append(i)
 return ("".join(data_elements)).encode("ascii")
def command_as_bytes(command):
 # In this implementation example, command is just a tuple or a list
  # containing (cmd, option, ...). This is not required, but shows you
 # how standard containers can also be suited for encapsulation.
 # You could instead declare
 # command_as_bytes(cmd, option=None, *option_args)
 cmd, option, *option_args = command
 
 # Validation
 if (not isinstance(cmd, Commands)):
 raise TypeError("invalid Telnet command")
 if (not isinstance(option, Options)):
 raise TypeError("invalid Telnet option")
 
 data = [IAC, cmd.value]
 
 if option is not None:
 data.append(option.value)
 data += [optarg.value for optarg in option_args]
 
 return bytes(data)

Be lazy. Delay actions till you are sure you (will) need them.
— A stupid quote, Me

Source Link

answered Feb 9, 2016 at 0:55

Cilyan

answered Feb 9, 2016 at 0:55

Cilyan

Q&A

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).
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 "<{members}>".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 can use an invalidation mechanism.

class Command:
 def __init__(...):
 # ...
 self._bytes_valid = False
 self._bytes = b''
 
 def __bytes__(self):
 # Recalc on access if necessary (cache-like)
 if not self._bytes_valid:
 self._bytes = self._calc_bytes()
 self._bytes_valid = True
 return self._bytes
 def __setattr__(self, name, value):
 super().__setattr__(name, value)
 self._bytes_valid = False

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

self.__dict__["cmd"] = cmd
self.__dict__["option"] = option

No.

self.cmd = cmd
self.option = option

Yes.

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 complexity

At its current state, your code is too big to do that few. There is no point in using classes to do the job.

def userdata_as_bytes(data):
 data_elements = []
 for i in data:
 if isinstance(i, CtrlChars):
 data_elements.append(i.value)
 else:
 data_elements.append(i)
 return ("".join(data_elements)).encode("ascii")
def command_as_bytes(command):
 # command is just a list or tuple (cmd, option, ...)
 cmd, option, *option_args = command
 
 # Validation
 if (not isinstance(cmd, Commands)):
 raise TypeError("invalid Telnet command")
 if (not isinstance(option, Options)):
 raise TypeError("invalid Telnet option")
 
 data = [IAC, cmd.value]
 
 if option is not None:
 data.append(option.value)
 data += [optarg.value for optarg in option_args]
 
 return bytes(data)

lang-py