I submitted a project and never heard back and I am wondering if my coding style is okay. At the company I work I get feedback, but I'd like to hear from the outside world. It's a drawing tool - the input is in a text file (each on its own line):

C 20 4

L 1 2 6 2

L 6 3 6 4

R 16 1 20 3

It's called as so: python drawingtool.py < input.txt

C = Canvas create, L = Line, R = Rectangle

from collections import defaultdict
import logging
import sys

NEWLINE = '\n'
DATA_SEP = '/'

class Canvas(object):
    _created = 0
    def __init__(self, cols, rows):
        self.rows = rows
        self.cols = cols
        Canvas._created += 1
        self.surface = self._create_blank_canvas()

    def _create_blank_canvas(self):
        row_frame_char = '-'
        col_frame_char = '|'
        canvas = []
        top_bottom = [row_frame_char for col in xrange(self.cols+2)]
        middle = [self._middle_char(col, col_frame_char, self.cols+1) for col in xrange(self.cols+2)]
        for row in xrange(self.rows+2):
            if row == 0 or row == self.rows+2:
        return canvas

    def _middle_char(self, col, col_frame_char, max):
        return col_frame_char if col == 0 or col == max else ' '

    def print_canvas(self):
        for row in self.surface:
            print ' '.join(row)

    def valid_pos(self, x, y):
        ret_val = False
        if not self._boundary(x, y) and self._on_canvas(x, y):
            ret_val = True
        return ret_val

    def _boundary(self, x, y):
        return (x == 0 or x == self.rows+1) or (y == 0 or y == self.cols+1)

    def _on_canvas(self, x, y):
        return (x > 0 and x < self.cols+1) or (y > 0 or y < self.rows+1)

    def update_cell(self, y, x, val=None):
        if self.valid_pos(y, x):
            self.surface[int(y)][int(x)] = val #[int(1)]
            logging.warning('Invalid position found x=%s y=%s' % (x,y))

class DataUtils(object):
    def __init__(self):
        self.raw_data = None

    def prepare_data(self):
        return self.apply_strip(self.raw_data, '\n')

    def split_up_by_delimiter(self, data, delim):
        return [filter(lambda x:x, map(lambda x:x, y.split(delim))) for y in data]

    def read_data_lines(self):
        return sys.stdin.readlines()

    def write_data_lines(self, data):

    def _sys_write(self, data):
        return sys.stdout.write(data)

    def _strip(self, text, char):
        return text.strip(char)

    def apply_strip(self, array_data, char):
        return map(lambda x:self._strip(x, char), array_data)

class Drawing_data(DataUtils):
    def __init__(self):
        self.raw_data = self.read_data_lines()

def process_q(command_q):
    attempts = 0
    canvas = None
    while command_q and attempts < MAX_ATTEMPTS:

        inspect_command = command_q[0]
        command = inspect_command[1].split(' ')[0]
        if not Canvas._created and command <> 'C':
            attempts += 1
            logging.error('Cant draw no canvas created skipping') #args[0].split(' ')
            request = command_q.pop(0)
            function, data = request[0], request[1].split(' ')

            if command == 'C' and Canvas._created == 0:
                canvas = request[0](data)
                request[0](data, canvas)

        if Canvas._created > 0:

# all calls expect the whole command line
def C(args):
    canvas = Canvas(int(args[1]), int(args[2]))
    return canvas

def L(args, canvas):
    _, startx, starty, finishx, finishy = args
    direction_x = False
    var, fixed, stop, increment = 0, 0, 0, 1
    #if canvas.valid_pos(startx, starty) and canvas.valid_pos(finishx, finishy):
    if (int(startx) - int(finishx)): #x is not fixed, y is fixed
        increment, var = (-1, int(startx)) if int(finishx) - int(startx) < 0 else (1, int(startx))
        fixed = starty
        stop = abs(int(finishx) - var) + int(startx)
    else: #x is fixed, y is not fixed
        increment, var = (-1, int(starty)) if int(finishy) - int(starty) < 0 else (1, int(starty))
        fixed = startx
        stop = abs(int(finishy) - var) + int(starty)
        direction_x = True

    while var <= stop:
        if direction_x:
            canvas.update_cell(var, fixed, 'x')
            canvas.update_cell(fixed, var, 'x')
        var += increment

def R(args, canvas):
    _, startx, starty, finishx, finishy = args
    L(['L', startx, starty, startx, finishy], canvas)
    L(['L', startx, starty, finishx, starty], canvas)
    L(['L', startx, finishy, finishx, finishy], canvas)
    L(['L', finishx, starty, finishx, finishy], canvas)

def B(args, canvas):

def unhandled(args, canvas):
    logging.error('unhandled command:%s for canvas %s' % (args, canvas))

if __name__ == '__main__':
    drawing_data = Drawing_data()
    drawing_data = drawing_data.prepare_data()
    dispatcher = {'C':C, 'L':L, 'R':R, 'B':B}
    command_q = []
    for data in drawing_data:
        command = dispatcher.get(data[0], unhandled)


1 Answer 1


First off, there should be two blank lines between top-level functions/classes/code blocks. You've also styled the class Drawing_data. It should be appropriately renamed to DrawingData.

Secondly, the class Drawing_data is completely pointless. Is there any reason why self.data = self.read_data_lines() can't be part of the DataUtils class?

While I understand your logic behind naming your actual drawing functions C, L, R, and B, it'd be better if you gave them better names, like canvas, and line

You also shouldn't be using % for string formatting. If you're running Python 2.6 or higher, string formatting with % is deprecated, and str.format should be used instead. Here's an example of how to use str.format:

# str.format without positional or named arguments
print "{} {}".format("Hello", "world")

# str.format with positional arguments
print "{1} {0}".format("world", "Hello")

# str.format with named arguments
print "{word1} {word2}".format(word1="Hello", word2="world")

I'm also noticing a pattern with your inputted commands. The always form a structure somewhat like this: [name] [integer] .... While it works with a small example like this, if you have bigger commands, I'd recommend giving them actual command names, and doing something like this: canvas width:20 height:20. It makes things a lot clearer to read.

Finally, I'd recommend adding docstrings to your code. Right now, nothing in your code file has docstrings. Here's an example of docstrings in use:

This docstring is at the top of the file,
it should describe the purpose of this file.

class MyClass(object):
    Describe your class and it's arguments here.

def my_func( ... ):
    Describe your function and it's arguments here.
  • \$\begingroup\$ @Michael No problem! Always willing to help out! :) \$\endgroup\$ Commented Jul 11, 2015 at 22:25
  • \$\begingroup\$ With your feedback, I've refactored much of the logic to standardize the row, col references and simplified the logic around making the lines. \$\endgroup\$
    – Michael
    Commented Jul 14, 2015 at 12:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.