patternpythonMinor
Python ASCII-Art Table
Viewed 0 times
tableartasciipython
Problem
As a part of a console utilities module, I created a function that takes a table in the form of an array of arrays, and generates an ASCII table with the given contents.
I've also added the options of adding a header, determining the in-cell alignment, and adding a border to the table.
Table cells have auto computed lengths by the longest cell of their column, rows that have a small amount of elements get filled by blank cells to adjust their length.
I'm quite new to Python, so I wanted to know about whether my writing style is OK. Can I improve or shorten some parts of the code? Any other advice?
Demo:
`>>> from asciiart import ascii_table
>>> table = list(map(lambda x: x.split(), ['Jon Doe 20', 'Mark Waine 35', 'Donald Rory 43']))
>>> header = ['First Name', 'Las
I've also added the options of adding a header, determining the in-cell alignment, and adding a border to the table.
Table cells have auto computed lengths by the longest cell of their column, rows that have a small amount of elements get filled by blank cells to adjust their length.
I'm quite new to Python, so I wanted to know about whether my writing style is OK. Can I improve or shorten some parts of the code? Any other advice?
def ascii_table (table, **k):
header = k.get('header', [])
align = k.get('align', 'left')
border = k.get('border', False)
widths = []
for i in range(max(map(len, table))): widths.append(max(max(map(len, [row[i] for row in table if len(row) > i])), len(header[i]) if len(header) > i else 0))
printable = []
if border:
printrow = []
for i in range(max(map(len, table))):
if i > 0 and i 0:
printrow = []
for i in range(len(header)):
assert header[i]
if align == 'center': printrow.append(header[i].center(widths[i]))
elif align == 'left': printrow.append(header[i].ljust(widths[i]))
elif align == 'right': printrow.append(header[i].rjust(widths[i]))
if border: printable.append('│ ' + ' │ '.join(printrow) + ' │')
else: printable.append(' │ '.join(printrow))
printrow = []
for i in range(len(header)):
if i > 0 and i 0 and i < max(map(len, table)) - 1: printrow.append('─' * (widths[i] + 2))
else: printrow.append('─' * (widths[i] + 1))
printable.append('└─' + '┴'.join(printrow) + '─┘')
return '\n'.join(printable)Demo:
`>>> from asciiart import ascii_table
>>> table = list(map(lambda x: x.split(), ['Jon Doe 20', 'Mark Waine 35', 'Donald Rory 43']))
>>> header = ['First Name', 'Las
Solution
In no particular order:
-
Your approach to default keyword arguments with the
Note that I’ve dropped the space between the function name and the opening parens (per PEP 8 style), and also used
This style is more in keeping with other Python code, and makes it easier for somebody to see what parameters this function takes. It also makes it easier to spot when somebody is calling this with entirely wrong arguments.
-
Newlines are cheap! Don’t put stuff on a single line, especially
would usually be written more like:
That style is more conventional, and makes it easier to see the structure of the code.
-
Loop over the elements of a list directly, not its indices. For example, this loop:
could be better written as something like:
Iterating directly over lists is more Pythonic, and generally makes for cleaner code.
-
Don’t cram too much into a single expression; it becomes difficult to parse and hurts readability. This is on the edge of what I’d consider acceptable:
This is unreadable:
You should break it across multiple lines.
-
You can tidy up some of your logical conditions. For example, this:
can be reduced to:
which is a bit easier to read.
-
This could potentially choke on a large table, because you’re building all the rows in a list
Then your function just lazily computes the rows, and passes them to the caller. It’s up to them how they want to use it – whether they want to save it, do some more processing, join it into a string (and then they can choose the newline).
Generators and iteration are a very powerful feature of Python – if you’re not familiar with them already, this PyCon talk is a good introduction.
-
Your approach to default keyword arguments with the
**k is unusual. I’d expect to see something more like: def ascii_table(table, header=None, align='left', border=False):
if header is None:
header = []Note that I’ve dropped the space between the function name and the opening parens (per PEP 8 style), and also used
None as the default for the header parameter – this is to avoid quirks around mutable default arguments.This style is more in keeping with other Python code, and makes it easier for somebody to see what parameters this function takes. It also makes it easier to spot when somebody is calling this with entirely wrong arguments.
-
Newlines are cheap! Don’t put stuff on a single line, especially
if … else blocks. For example, this block: if border: printable.append('│ ' + ' │ '.join(printrow) + ' │')
else: printable.append(' │ '.join(print row))would usually be written more like:
if border:
printable.append('│ ' + ' │ '.join(printrow) + ' │')
else:
printable.append(' │ '.join(print row))That style is more conventional, and makes it easier to see the structure of the code.
-
Loop over the elements of a list directly, not its indices. For example, this loop:
for i in range(len(header)):
assert header[i]
if align == 'center':
printrow.append(header[i].center(widths[i]))
....
# do stuff with header[i]could be better written as something like:
for h, w in zip(header, widths):
if align == 'centre':
printrow.append(h.center(w))
...Iterating directly over lists is more Pythonic, and generally makes for cleaner code.
-
Don’t cram too much into a single expression; it becomes difficult to parse and hurts readability. This is on the edge of what I’d consider acceptable:
for i in range(max(map(len, table))):This is unreadable:
for i in range(max(map(len, table))): widths.append(max(max(map(len, [row[i] for row in table if len(row) > i])), len(header[i]) if len(header) > i else 0))You should break it across multiple lines.
-
You can tidy up some of your logical conditions. For example, this:
if i > 0 and i < max(map(len, table)) - 1:can be reduced to:
if 0 < i < max(len(t) for t in table) - 1which is a bit easier to read.
-
This could potentially choke on a large table, because you’re building all the rows in a list
printable. I would consider rewriting this as a generator – essentially, replace every printable.append(foo) with yield foo.Then your function just lazily computes the rows, and passes them to the caller. It’s up to them how they want to use it – whether they want to save it, do some more processing, join it into a string (and then they can choose the newline).
Generators and iteration are a very powerful feature of Python – if you’re not familiar with them already, this PyCon talk is a good introduction.
Code Snippets
def ascii_table(table, header=None, align='left', border=False):
if header is None:
header = []if border: printable.append('│ ' + ' │ '.join(printrow) + ' │')
else: printable.append(' │ '.join(print row))if border:
printable.append('│ ' + ' │ '.join(printrow) + ' │')
else:
printable.append(' │ '.join(print row))for i in range(len(header)):
assert header[i]
if align == 'center':
printrow.append(header[i].center(widths[i]))
....
# do stuff with header[i]for h, w in zip(header, widths):
if align == 'centre':
printrow.append(h.center(w))
...Context
StackExchange Code Review Q#144755, answer score: 8
Revisions (0)
No revisions yet.