Неоднозначное использование переменных и слишком много знаний при формировании SVG


Исходный код

if isinstance(labels, str):
    try:
        labels = self.vs.get_attribute_values(labels)
    except KeyError:
        labels = [x+1for x in xrange(self.vcount())]
elif labels isNone:
    labels = [""] * self.vcount()

if isinstance(colors, str):
    try:
        colors = self.vs.get_attribute_values(colors)
    except KeyError:
        colors = ["red"for x in xrange(self.vcount())]

if isinstance(shapes, str):
    try:
        shapes = self.vs.get_attribute_values(shapes)
    except KeyError:
        shapes = [1]*self.vcount()

if isinstance(edge_colors, str):
    try:
        edge_colors = self.es.get_attribute_values(edge_colors)
    except KeyError:
        edge_colors = ["black"for x in xrange(self.ecount())]

if not isinstance(font_size, str):
    font_size = "%spx" % str(font_size)
else:
    if ";" in font_size:
        raise ValueError, "font size can't contain a semicolon"

vc = self.vcount()
while len(labels)<vc: labels.append(len(labels)+1)
while len(colors)<vc: colors.append("red")

#.........
for eidx, edge in enumerate(self.es):
    print >>f, "    <g transform=\"translate(%.4f,%.4f)\" fill=\"%s\" stroke=\"%s\">" % (x2, y2, edge_colors[eidx], edge_colors[eidx]

Что не так в исходном коде

Неоднозначное использование переменных

Переменные labelscolorsshapesedge_colors используются в двух смыслах: как массив значений некоего свойства, либо как строковый ключ в хранилище, из которого этот массив извлекается.

Такая двусмысленность не годится: она затрудняет понимание алгоритма, резко повышает вероятность багов и затрудняет их устранение.

Для каждого свойства нужно завести пару переменных: отдельную для массива значений свойств и отдельную — для ключа, по которому значения свойств извлекаются из хранилища.

Смешанные знания

В одной простыне кода смешаны знания о том, как формировать массивы значений свойств для меток, цвета, формы и цвета границы, размера шрифта, а затем и формирования фигур на основе сформированных массивов.

Более здоровый подход — разместить код для формирования каждого набора данных, которые здесь выглядят совершенно самостоятельными, в самостоятельные же методы, чтобы каждый метод знал только одну вещь: как сформировать значения цвета фигур, как сформировать метки фигур, как сформировать фигуры и т. д.

Невнятные названия полей

Названия self.vsself.vcountself.esself.ecount совершенно ничего не сообщают о своём предназначении, для его выяснения придётся разгрести много другого кода.

Сомнительная архитектура

Думаю, не сильно ошибусь, если предположу, что элементы массивов labelscolorsshapes соответствуют неким геометрическим объектам, а именно: labels[0]colors[0]shapes[0] — это свойства первого объекта, labels[0]colors[0]shapes[0] — второго и т. д. Если так, то очень правильно будет завести отдельный клас «геометрический объект» со свойствами «метка», «форма» и «цвет».

То же самое с edge_colors, который относится к группам объектов, судя по тегу <g>, с помощью которого они рендерятся.

Выявление в своей предметной области сущностей и создание под них самостоятельных классов позволяет проделывать массу удивительных вещей, в частности, правильно распределять по коду знания о том, как реализуются различные моменты в проекте.

Теория