Эта поделка умудрилась даже попасть в одну из статей журнала «Хакер» — что-то там было про brute forcing для VoIP-серверов. Автор обращался за разрешением, которое я ему и дал, предупредив что скорее всего можно лучше.
Code Style
В этой части больше придирки к стилю кода. И так, оригинал с некоторыми комментариями:
# -*- coding: utf-8 -*-
# Review: строку выше, оказывается, можно заменить на просто на
# «coding: utf-8», без всяких звёздочек и дефисов.
# Review: некоторые статические анализаторы кода (pylint в частности)
# считают что переменные на уровне модуля должны именоваться в верхнем
# регистре. Более того, состоять из 3 символов минимум. В общем-то,
# что значить переменная «p» — достаточно трудно догадаться,
# PUCNTUATION или PUNCT — было бы лучше.
EN = «ABCDEFGHIJKLMNOPQRSTUVWXYZ»
en = «abcdefghijklmnopqrstuvwxyz»
RU = «АБВГДЕЁЖЗИЙКЛМНОПРСТУФХЦЧШЩЪЫЬЭЮЯ»
ru = «абвгдеёжзийклмнопрстуфхцчшщъыьэюя»
digits = «1234567890»
space = » »
p = «,.-!?;:’\»/()»
op = «+-*/:^()<>=»
all_spec = «`~!@#$%^&*-_=+\\|/?.>,< ‘\»;:[]{}» # Review: перед объявлением класса должно быть дву пустых строки # (PEP8). Также, тут используется old-style класс, лучше наследоваться # от object. class ABCIterator: # Review: first_usage или first_usage_flag смотрелось бы органичней. # А _first_usage было бы ещё лучше, т.к. по сути переменная для # внутренеего пользования. firstuse_flag = True stop = None # Review: лучше соблюдать ограничение длины строки в 79 символов # (PEP8). def __init__(self, start=»», stop=None, start_len=None, stop_len=None, abc=en+EN+digits+all_spec): # Review: assert — это только для отладки. Лучше было бы # if …: raise ValueError(‘abc must be non empty’). # abc — должен содержать только уникальные значения, поэтому # лучше использовать frozenset или специальную реализацию # OrderedSet (нет в built-in, но гуглится легко). assert len(abc) > 0
if start_len is not None:
# Review: опять assert и лишний закомменченынй кусок.
assert start_len > 0
#assert start == «»
# Review: по PEP8 должны быть пробелы между операндами.
self.current_str = list(abc[0]*start_len)
else:
# Review: [x for x in start if x in abc]. Но если в
# стартовой строке символы не из допустимого алфавита, то
# лучше кинуть exception, не умничать и не прогибаться под
# неправильное использование.
self.current_str = list(filter(lambda x: x in abc, start))
if stop_len is not None:
# Review: опять assert.
assert (start_len is None) or (start_len <= stop_len) assert stop_len > 0
# Review: пробелы между операндами, 79 символов, 2 пробела
# перед in-line комментариями.
self.stop = list(abc[0]*(stop_len+1)) # т.к. итератор работает с полуотрезками
else:
# Review: лучше не лепить в одну строку. filter опять можно
# заменить на списковое выражение. 79 символов. И опять
# лучше кинуть exception.
if stop is not None: self.stop = list(filter(lambda x: x in abc, stop))
self.abc = list(abc)
def __iter__(self):
return self
def next(self):
if (self.stop is not None) and (self.stop == self.current_str):
raise StopIteration
# Review: наверное лучше было бы проверять длину.
if self.current_str == []:
self.current_str.append(self.abc[0])
self.firstuse_flag = False
return self.abc[0]
elif self.firstuse_flag:
self.firstuse_flag = False
return «».join(self.current_str)
# Review: цикл можно было бы сделать по current_str с
# использованием функции enumerate.
offset = 0
while offset < len(self.current_str): # Review: self.current_str[offset] используется 4 раза, # было бы удобней завести переменную для быстрого доступа. if self.current_str[offset] != self.abc[-1]: # Review: 79 символов, пробелы. self.current_str[offset] = self.abc[self.abc.index(self.current_str[offset])+1] # выпендрёшь для полуотрезка if (self.stop is not None) and (self.current_str == self.stop): raise StopIteration return «».join(self.current_str) self.current_str[offset] = self.abc[0] offset += 1 # Review: self.current_str.insert(0, self.abc[0]). Хотя вставка # справа должна работать скорее, если не ошибаюсь. Поэтому, # возможно, было бы удобней держать текущую строку в # реверсивном виде. self.current_str = [self.abc[0]] + self.current_str if (self.stop is not None) and (self.current_str == self.stop): raise StopIteration return «».join(self.current_str)[/code] Также, можно было бы обойтись генератором с использованием yield вместо итератора, в данном случае он лишний. На самом деле, оригинальная задумка была реализовать простой интерфейс с возможностью деления последовательностей на части, тут бы итератор был уместен, но в текущем варианте вполне можно обойтись и генератором.
Refactoring, v1
С учётом замечаний по стилю:
| 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 | # coding: utf-8 EN_UPPER = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" EN_LOWER = "abcdefghijklmnopqrstuvwxyz" RU_UPPER = "АБВГДЕЁЖЗИЙКЛМНОПРСТУФХЦЧШЩЪЫЬЭЮЯ" RU_LOWER = "абвгдеёжзийклмнопрстуфхцчшщъыьэюя" DIGITS = "1234567890" SPACE = " " PUNCTUATIONS = ",.-!?;:’\"/()" OPERATORS = "+-*/:^()<>=" ALL_SPECS = "`~!@#$%^&*-_=+\\|/?.>,< '\";:[]{}" DEFAULT_ABC = EN_LOWER + EN_UPPER + DIGITS + ALL_SPECS class ABCIterator(object): def __init__(self, start="", stop=None, start_len=None, stop_len=None, abc=DEFAULT_ABC): self._first_usage = True self.abc = tuple(abc) # FIXME: quick check for uniqueness, could be better if not abc or len(frozenset(abc)) != len(abc): raise ValueError('invalid abc') if start_len is not None: if start_len < 0: raise ValueError('invalid start len') self.current_str = list(abc[0] * start_len) else: self._check_str(start) self.current_str = start if stop_len is not None: # Review: опять assert. if start_len is not None and start_len > stop_len or stop_len < 0: raise ValueError('invalid stop_len') # т.к. итератор работает с полуотрезками, прибавляем 1 self.stop = list(abc[0] * (stop_len + 1)) else: if stop is not None: self._check_str(stop) self.stop = stop def _check_str(self, str_): """Check that str_ is allowed for current abc.""" if not hasattr(self, '_abc_set'): self._abc_set = frozenset(self.abc) if not self._abc_set.issuperset(str_): raise ValueError('invalid start string for given abc') def __iter__(self): return self def next(self): if (self.stop is not None) and (self.stop == self.current_str): raise StopIteration() if not self.current_str: self.current_str = [self.abc[0]] self._first_usage = False return self.current_str[0] elif self._first_usage: self._first_usage = False return "".join(self.current_str) for offset, value in enumerate(self.current_str): if value != self.abc[-1]: self.current_str[offset] = self.abc[self.abc.index(value) + 1] if (self.stop is not None) and (self.current_str == self.stop): raise StopIteration() return "".join(self.current_str) self.current_str[offset] = self.abc[0] self.current_str.insert(0, self.abc[0]) # опять же оно же if (self.stop is not None) and (self.current_str == self.stop): raise StopIteration() return "".join(self.current_str) |
Refactoring, v2
Итератор вместо генератора:
| 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 | # coding: utf-8 EN_UPPER = "ABCDEFGHIJKLMNOPQRSTUVWXYZ" EN_LOWER = "abcdefghijklmnopqrstuvwxyz" RU_UPPER = "АБВГДЕЁЖЗИЙКЛМНОПРСТУФХЦЧШЩЪЫЬЭЮЯ" RU_LOWER = "абвгдеёжзийклмнопрстуфхцчшщъыьэюя" DIGITS = "1234567890" SPACE = " " PUNCTUATIONS = ",.-!?;:’\"/()" OPERATORS = "+-*/:^()<>=" ALL_SPECS = "`~!@#$%^&*-_=+\\|/?.>,< '\";:[]{}" DEFAULT_ABC = EN_LOWER + EN_UPPER + DIGITS + ALL_SPECS def _check_str(str_, abc): """Check that str_ is allowed for current abc.""" if not frozenset(abc).issuperset(str_): raise ValueError('invalid start string for given abc') def abc_iter(start="", stop=None, start_len=None, stop_len=None, abc=DEFAULT_ABC): # prepearing stuff abc = tuple(abc) # FIXME: quick check for uniqueness, could be better if not abc or len(frozenset(abc)) != len(abc): raise ValueError('invalid abc') if start_len is not None: if start_len < 0: raise ValueError('invalid start len') current_str = list(abc[0] * start_len) else: _check_str(start, abc) current_str = start if stop_len is not None: # Review: опять assert. if start_len is not None and start_len > stop_len or stop_len < 0: raise ValueError('invalid stop_len') # т.к. итератор работает с полуотрезками, прибавляем 1 stop = list(abc[0] * (stop_len + 1)) else: if stop is not None: _check_str(stop, abc) stop = stop # start iterations if not current_str: current_str = [abc[0]] while True: for offset, value in enumerate(current_str): if stop is not None and current_str == stop: raise StopIteration() yield ''.join(current_str) if value != abc[-1]: current_str[offset] = abc[abc.index(value) + 1] else: current_str[offset] = abc[0] if stop is not None and current_str == stop: raise StopIteration() yield ''.join(current_str) # we can't just insert new first value from abc because then # we would miss all-first combination (e.g. '0000' for digits) current_str = list(abc[0] * (len(current_str) + 1)) |
Refactoring, v3
На самом деле больше кода не будет, просто хотел упомянуть что можно было реализовать часть подобного функционала с помощью itertools.product, как было сказано в комментариях к оригиналу.
P.S. Переделывал код достаточно поздно, поэтому, может где-то ошибся. Но на первый взгляд всё в порядке.
P.P.S. Понял что подсветка кода немного корявая и портит исходники, поэтому часть кода без неё.
