Obvious but easy
There are some issues that are immediately obvious and easy to correct.
In this statement:
return ''.join([repr(a), '-', repr(b - 1)]) if a != b else repr(b)
The multiple repr
are awkward,
and so is the join
.
It would be less awkward, simpler and more readable by using string formatting:
return '{}-{}'.format(a, b - 1) if a != b else repr(a)
This can be written a lot simpler:
if char in ['(', '{', '[', ')', '<', '}', ']', '>']:
Like this:
if char in '({[)<}]>':
But actually, it's a bit disturbing that the opening and closing brackets are ordered inconsistently. For example, looking at this string, it's not immediately obvious that it includes all matching closing brackets for all opening brackets. This way would be immediately obvious and less error prone:
if char in '(){}[]<>':
This is both confusing and disturbing:
def convert_string(string):
string_content = string
string = Positions(string)
Why shuffle around the string
parameter?
It's a bad practice to reassign the value of a parameter,
as it makes it confusing what it really stands for throughout the function.
Another problem here is calling an instance of Positions
a "string".
Later in the code when I read "string",
I think it's an actually string, but in fact it's not,
it's an instance of "Positions".
Very misleading.
Since empty strings are falsy, instead of this:
if internal_string != '':
# ...
You can write simply:
if internal_string:
# ...
Obvious but harder
There are some issues that are sort of obviously wrong,
but not so easy to correct.
This sorting function smells:
def sort(dictionary):
return sorted(dictionary.items(), key=lambda v: int(v[0].split("-")[0]))
Instead of splitting a string and parsing to int,
my instinct says there should be a way to use the original int values directly.
In general, the code formats these strings from int values,
and it seems silly to later un-format the strings so you can sort.
There should be a better way.
The Positions
class stinks.
It's called "Positions",
but its fields are "name" and "content".
Sounds more like a key-value pair.
Then there's this code with the comment:
self.content = {} # creates a new empty list for each bracket
The comment talks about "list", but the code uses a dictionary.
And there's the push
method:
def push(self, pos, content):
self.content[pos] = content
push
is a term commonly used with stacks, or sometimes with lists.
But here the method puts a value into a dictionary,
an operation normally called "put".
Alternative implementation
Based on the above suggestions,
and reorganizing to more meaningful elements,
you could do something like this:
from functools import total_ordering
@total_ordering
class Item:
def __init__(self, start, end, content):
self.start = start
self.end = end
self.content = content
@property
def position(self):
a = self.start
b = self.end
return '{}-{}'.format(a, b - 1) if a != b else repr(a)
def __lt__(self, other):
return self.start < other.start
def convert_string(string):
start = 0
internal_string = ''
items = []
for (pos, char) in enumerate(string):
if char in '(){}[]<>':
items.append(Item(pos, pos, char))
if internal_string:
items.append(Item(start, pos, internal_string))
internal_string = ''
start = pos + 1
else:
internal_string += char
if internal_string:
items.append(Item(start, len(string), internal_string))
return items
Although with the implementation of __lt__
,
I made it possible to sort simply by sorted(items)
,
actually there is no need to sort,
as the items are naturally sorted.
Note: the purpose of @total_ordering
is to implement the other rich comparison operators like __le__
, as recommended by PEP8.
lowercase_with_underscore
function/variable names, so you're not yet fully compliant. \$\endgroup\$