5
\$\begingroup\$

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?

\$\endgroup\$
6
\$\begingroup\$
#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()
| improve this answer | |
\$\endgroup\$
  • \$\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\$ – Alexander Abashkin Feb 7 '12 at 17:21
2
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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