Recently I had the idea to write a module for the transformation of the dictionary into valid CSS code. And I would like to hear your comments on my code. License MIT, BSD etc.
The code is very simple:
#pss.py
class PSS:
def __init__(self, obj):
self.obj = obj
self.__data = {}
self.__parse(obj)
def __repr__(self):
return self.__build(self.obj)
def __build(self, obj, string = ''):
for key, value in sorted(self.__data.items()):
if self.__data[key]:
string += key[1:] + ' {\n' + ''.join(value) + '}\n\n'
return string
def __parse(self, obj, selector = ''):
for key, value in obj.items():
if hasattr(value, 'items'):
rule = selector + ' ' + key
self.__data[rule] = []
self.__parse(value, rule)
else:
prop = self.__data[selector]
prop.append('\t%s: %s;\n' % (key, value))
import module:
#test.py
from pss import *
css = PSS({
'html': {
'body': {
'color': 'red',
'div': {
'color': 'green',
'border': '1px'
}
}
}
})
print(css)
Result:
html body {
color: red;
}
html body div {
color: green;
border: 1px;
}
So I need your advice to improve the quality of the code. Perhaps there are still places that can be done easier?
2 Answers 2
#pss.py
I'd recommend an actual docstring rather then a comment
__version__ = '1.0'
class PSS:
def __init__(self, obj):
if type(obj) is not dict:
raise RuntimeError('There\'s no any objects to parse!')
What if its a subclass of dict? What if its a dict-like object? In python we generally do not check types. So I'd just remove this bit. I'd also name it something more meaningful then obj
self.obj = obj
self.__data = {}
Use of __
is controversial. I'm in the camp that you should only use one undescore.
def __repr__(self):
return self.__build(self.obj)
This isn't what __repr__
is for. __repr__
should give a python representation of your object, not produce the string version. Look at repr() vs str() on python strings.
def __build(self, obj, string = ''):
self.__parse(obj)
Why parse here instead of in the constructor?
for i in sorted(self.__data.keys()):
i
is a terrible name, it usually means index but you aren't using it that way here
if self.__data[i]:
Use for key, value in sorted(self.__data.items()):
then you won't need to access self.__data[i]
string += i[1:] + ' {\n' + ''.join(self.__data[i]) + '}\n\n'
Adding strings can be expensive. Its generally better to build the strings in a list and then join them or use StringIO, and write the string pieces by piece.
return string
def __parse(self, obj, selector = ''):
for i in obj:
use for key, value in obj.items()
whenever you need both the keys and values in a dict. Python isn't stupid like some other languages
if type(obj[i]) is dict:
Checking types is considered bad form in python. If you must differentiate between types it is perferred to check some aspect of the dict interface, such as the items method.
rule = selector + ' ' + i
I'd store these as tuples rather then strings.
self.__data[rule] = []
self.__parse(obj[i], rule)
else:
prop = self.__data[selector]
prop.append('\t%s: %s\n' % (i, obj[i]))
Again, I'd store this as something besides a string. Probably keys in a dictionary.
return self
Why return self
Parsing usually refers to converting text into some internal representation. But you are actually doing the opposite. You are converting python objects into partial python objects, partial text. That's simply not parsing. Its also not clear that you gain anything by doing this pre-processing before converting entirely over to text.
from pss import *
css = PSS({
'html': {
'body': {
'color': 'red',
'div': {
'color': 'green',
'border': '1px'
}
}
}
})
I must admit that looks pretty sweet.
Interface-wise, why is this a class? It seems to me that is should be a function. Its a one way transformation from dicts to css string representation. That seems to me to be a fit for a function not a class.
Here is my tackle at the problem:
from StringIO import StringIO
def write_css(selector, data, output):
children = []
attributes = []
for key, value in data.items():
if hasattr(value, 'items'):
children.append( (key, value) )
else:
attributes.append( (key, value) )
if attributes:
print >> output, ' '.join(selector), "{"
for key, value in attributes:
print >> output, "\t", key + ":", value
print >> output, "}"
for key, value in children:
write_css(selector + (key,), value, output)
def PSS(data):
output = StringIO()
write_css((), data, output)
return output.getvalue()
-
\$\begingroup\$ Wow, More thanks! I've considered all your comments and try to avoid more of these errors. In the future I will try to extend the functionality of the one.
return self
is a typo, sorry ` \$\endgroup\$WayHunter– WayHunter2012年02月07日 17:21:05 +00:00Commented Feb 7, 2012 at 17:21
Two random remarks:
"__parse" isn't a great method name since no actual parsing is going on.
for i in sorted(self.__data.keys()):
The variable name "i" is commonly used for (list) indexes. When iterating over a dict, it's more clear to use "k", "key" or (preferably) something more descriptive.