4
\$\begingroup\$

I'm trying to learn Python and am working on a simple Tiled Map Format (.tmx) reader as practice. So as to not post too much code at once, I'm only publishing the <data> element which stores the tiles displayed in a tiled map.

I'm looking for any critique, no matter how harsh, on quality of code, the Pythonic way of coding, my unit tests, etc.

First, the description of the <data> element from the documentation:

<data>

encoding: The encoding used to encode the tile layer data. 
          When used, it can be "base64" and "csv" at the moment.
compression: The compression used to compress the tile layer data. 
             Supports "gzip" and "zlib".

First you need to base64-decode it, then you may need to decompress it. 
Now you have an array of bytes, which should be interpreted as an array of 
unsigned 32-bit integers using little-endian byte ordering.

Whatever format you choose for your layer data, 
you will always end up with so called "global tile IDs" (gids).

The code:

@value_equality
class Data:
    """
    Contains the information on how the current map is put together.

    Data is compressed first, then encoded. Then the data is an array of bytes which should be interpreted
    as an array of unsigned 32-bit integers in little-endian format.

    If neither encoding nor compression is used, then the data is stored as an xml list of tile elements.

    All formats represent global tile ids and may refer to any tilesets used by the map. In order to correctly map the
    global id to the tileset, find the tileset with the highest firstgid that is still lower or equal than the gid. The
    tilesets are always stored with increasing firstgids
    """

    @property
    def encoding(self):
        """
        The encoding used to encode the tile layer data.
        Can be either "base64" or "csv".
        (Optional)
        """
        self._encoding

    @property
    def compression(self):
        """
        The compression used to compress the tile layer data.
        Can be either "gzip" or "zlib"
        (Optional)
        """
        self._compression

    @property
    def tiles(self):
        self._tiles

    @property
    def global_tile_ids(self):
        self._global_tile_ids

    def __init__(self, encoding=None, compression=None, global_tile_ids=[], tiles=None):
        self._encoding = encoding
        self._compression = compression
        self._global_tile_ids = global_tile_ids
        self._tiles = tiles

    def __eq__(self, other):
        return self.__dict__ == other.__dict__ if isinstance(other, self.__class__) else False

    def __ne__(self, other):
        return not self.__eq__(other)

    @classmethod
    def from_tmx(cls, element):
        """Parse <data> element from .tmx file"""
        attributes = {
            'encoding': element.attrib['encoding'] if 'encoding' in element.attrib else None,
            'compression': element.attrib['compression'] if 'compression' in element.attrib else None,
        }

        tile_ids = []
        if attributes['encoding'] is None and attributes['compression'] is None:
            map(tile_ids.append, element.iter('tile'))
        else:
            decoded = Data._decode(element.text, attributes['encoding'])
            decompressed = Data._decompress(decoded, attributes['compression'])
            tile_ids = list(decompressed)

        attributes['global_tile_ids'] = list(map(DataTile.from_bytes, tile_ids))

        return cls(**attributes)

    @staticmethod
    def _decode(data, encoding):
        decoders = {
            'base64': (lambda x: base64.b64decode(x)),
            'csv': (lambda x: map(int, x.strip().split(','))),
            None: (lambda x: x)
        }

        return decoders[encoding](data)

    @staticmethod
    def _decompress(data, compression):
        decompressors = {
            'gzip': (lambda x: gzip.decompress(data)),
            'zlib': (lambda x: zlib.decompress(data)),
            None: (lambda x: x)
        }

        return decompressors[compression](data)

@value_equality
class DataTile():
    HORIZONTAL_FLIP_BIT = 0x80000000
    VERTICAL_FLIP_BIT   = 0x40000000
    DIAGONAL_FLIP_BIT   = 0x20000000

    @property
    def global_tile_id(self):
        return self._global_tile_id

    @property
    def flipped_horizontally(self):
        return self._flipped_horizontally

    @property
    def flipped_vertically(self):
        return self._flipped_vertically

    @property
    def flipped_diagonally(self):
        return self._flipped_diagonally

    def __init__(self, global_tile_id, flipped_horizontally=False, flipped_vertically=False, flipped_diagonally=False):
        self._global_tile_id = global_tile_id
        self._flipped_horizontally = flipped_horizontally
        self._flipped_vertically = flipped_vertically
        self._flipped_diagonally = flipped_diagonally

    def __eq__(self, other):
        return self.__dict__ == other.__dict__ if isinstance(other, self.__class__) else False

    def __ne__(self, other):
        return not self.__eq__(other)

    @classmethod
    def from_bytes(cls, tile_id):
        attributes = {
            'global_tile_id': tile_id & ~(cls.HORIZONTAL_FLIP_BIT | cls.VERTICAL_FLIP_BIT | cls.DIAGONAL_FLIP_BIT),
            'flipped_horizontally': (tile_id & cls.HORIZONTAL_FLIP_BIT != 0),
            'flipped_vertically': (tile_id & cls.VERTICAL_FLIP_BIT != 0),
            'flipped_diagonally': (tile_id & cls.DIAGONAL_FLIP_BIT != 0)
        }

        return cls(**attributes)

My first unit tests:

class TiledMapReaderTest(unittest.TestCase):

    def setUp(self):
        self._unknowntmx = self._resource('Unknown.tmx')
        self._completexml = self._resource('Complete.xml')
        self._minimaltmx = self._resource('Minimal.tmx')
        self._completetmx = self._resource('Complete.tmx')
        self._tilesettmx = self._resource('Tileset.tmx')
        self._imagetmx = self._resource('Image.tmx')
        self._datatmx = self._resource('Data.tmx')
        self._csvtmx = self._resource('CsvData.tmx')

    def _resource(self, name):
        return os.path.join(os.path.dirname(__file__), 'res', name)

    def _root(self, file):
        return ET.parse(file).getroot()

    # Snip unrelated tests

    def test_datatag_constructsdata(self):
        root = self._root(self._datatmx)

        actual = Data.from_tmx(root)
        expected = Data(encoding='base64', compression='gzip')
        self.assertEqual(expected._encoding, actual._encoding)
        self.assertEqual(expected._compression, actual._compression)

    def test_datatag_base64_decodesdata(self):
        data = b'H4sIAAAAAAAAAO3NoREAMAgEsLedAfafE4+s6l0jolNJiif18tt/Fj8AAMC9ARtYg28AEAAA'

        actual = Data._decode(data, 'base64')
        expected = base64.b64decode(data)
        self.assertEqual(expected, actual)

    def test_datatag_gzip_decompressesdata(self):
        data = b'H4sIAAAAAAAAAO3NoREAMAgEsLedAfafE4+s6l0jolNJiif18tt/Fj8AAMC9ARtYg28AEAAA'

        actual = Data._decompress(Data._decode(data, 'base64'), 'gzip')
        expected = gzip.decompress(base64.b64decode(data))
        self.assertEqual(expected, actual)

    def test_datatile_frombytes(self):
        flipped_horizontally = 0x80000001
        flipped_vertically = 0x40000002
        flipped_diagonally = 0x20000003
        flipped_everyway = 0xe0000004

        expected = DataTile(global_tile_id=1, flipped_horizontally=True)
        self._datatile_equal(flipped_horizontally, expected)

        expected = DataTile(global_tile_id=2, flipped_vertically=True)
        self._datatile_equal(flipped_vertically, expected)

        expected = DataTile(global_tile_id=3, flipped_diagonally=True)
        self._datatile_equal(flipped_diagonally, expected)

        expected = DataTile(global_tile_id=4, flipped_horizontally=True, flipped_vertically=True, flipped_diagonally=True)
        self._datatile_equal(flipped_everyway, expected)

    def _datatile_equal(self, bytes, expected):
        actual = DataTile.from_bytes(bytes)

        self.assertEqual(expected, actual)

Custom @value_equality decorator

def value_equality(cls):
    """Implement __eq__ and __ne__; caution, does not work with polymorphic yet."""
    cls.__eq__ = lambda self, other: self.__dict__ == other.__dict__ if isinstance(other, self.__class__) else False
    cls.__ne__ = lambda self, other: not self.__eq__(other)
    return cls
\$\endgroup\$
3
\$\begingroup\$

1. General comments

  1. You have a tendency to over-complicate things. As a result, your code is full of mistakes that you failed to spot. Keep code as simple as possible and it will be easier to inspect and test!

  2. The code you posted is missing a bunch of import statments needed to run. To help other reviewers, I think that these imports need to be:

    import base64
    import gzip
    import os
    import unittest
    import xml.etree.ElementTree as ET
    import zlib
    
  3. It is worth keeping to 79 columns as recommended by the Python style guide (PEP8). Your code is hard to read here on Code Review because the long lines don't fit and we have to scroll horizontally to read them.

2. value_equality decorator

  1. Your decorator value_equality does not work! It looks as though you have not tested it in the subclass case. Suppose we have:

    @value_equality
    class A: pass
    class B(A): pass
    

    then an object of the subclass B does not compare equal to an object of the superclass A if we use the == operator:

    >>> a, b = A(), B()
    >>> a == b
    False
    

    and yet the __eq__ method works fine:

    >>> a.__eq__(b)
    True
    

    The Python feature that explains this behaviour is a bit obscure, but it is documented, in this note about the implementation of special methods:

    Note: If the right operand’s type is a subclass of the left operand’s type and that subclass provides the reflected method for the operation, this method will be called before the left operand’s non-reflected method. This behavior allows subclasses to override their ancestors’ operations.

    Combined with the fact that __eq__ is its own reflected operation, this means that when Python evaluates a == b, it notes that the type of b is a subclass of the type of a, and so it calls b.__eq__(a), which tests isinstance(a, b.__class__) which is False.

    This highlights a basic problem with the design of value_equality. Equality should be a symmetric operation: that is, if a equals b then b should equal a, and so it shouldn't matter which way round the comparison is done. But in your implementation you use an isinstance test which is not symmetrical.

  2. I like that you've written docstrings. But unfortunately you've written them from the wrong point of view. The purpose of a docstring is to provide help to the user: that is, to answer questions like "what does this do?" and "how do I use it?". For example:

    >>> help(abs)
    Help on built-in function abs in module builtins:
    
    abs(...)
        abs(number) -> number
    
        Return the absolute value of the argument.
    

    But you've written your docstrings from the point of view of the implementor: basically they are notes to yourself that really should be comments instead. For example, the docstring for value_equality is

    Implement __eq__ and __ne__; caution, does not work with polymorphic yet.
    

    where the first clause is a detail of the implementation, and the second is just obscure to me. (See below for how the docstring should look.)

    Similarly, the docstring for the Data class contains a description of how the list of tiles is encoded into a <data> element. This is handy for you as an implementor, but is useless for the user of the Data class. Put this information in a comment (or better still, provide a link to the TMX documentation, which is more comprehensive).

  3. value_equality doesn't need to be a decorator because it does not look at the implementation of cls. So you should prefer to make it into a class instead. Classes are simpler to understand than decorators.

    class EqualAttributes(object):
        """Class of objects that can be compared for equality. Instances
        compare equal if their type and writable attributes are equal. For
        example:
    
            >>> class Foo(EqualAttributes): pass
            >>> a, b = Foo(), Foo()
            >>> a.x, b.x = 1, 1
            >>> a == b, a != b
            (True, False)
            >>> a.y, b.y = 1, 2
            >>> a == b, a != b
            (False, True)
    
        """
        def __eq__(self, other):
            return type(self) == type(other) and self.__dict__ == other.__dict__
    
        def __ne__(self, other):
            return not (self == other)
    

    Notes:

    • The docstring is written from the user's point of view.
    • There are some examples in the form of doctests.
    • I've used type(self) which is clearer than self.__class__.
    • I've addressed the symmetry problem by restricting equality to objects of the same type.
  4. The Data and DataTile classes have __eq__ and __ne__ methods, so why do they also have @value_equality? Surely they should have one or the other, but not both?

  5. Are you sure you really need the value_equality decorator at all? You only ever use it in your unit tests, so I think things would be much simpler if you just got rid of this class and compared attributes directly in the test cases.

3. DataTile class

  1. There's no docstring for the DataTile class. What does this class do and how should I use it?

  2. The name DataTile seems superfluous. Why not just Tile?

  3. Instead of:

    HORIZONTAL_FLIP_BIT = 0x80000000
    VERTICAL_FLIP_BIT   = 0x40000000
    DIAGONAL_FLIP_BIT   = 0x20000000
    

    I would write:

    HORIZONTAL_FLIP = 1 << 31
    VERTICAL_FLIP   = 1 << 30
    DIAGONAL_FLIP   = 1 << 29
    

    to make it clear where these numbers come from.

  4. The from_bytes method is mis-named: it doesn't actually take a sequence of bytes as an argument, but rather a tile id as a number.

  5. In the from_bytes method, it's pointless to build a dictionary and then immediately pass it as keyword arguments. Just pass the keyword arguments instead.

  6. The value ~(cls.HORIZONTAL_FLIP_BIT | cls.VERTICAL_FLIP_BIT | cls.DIAGONAL_FLIP_BIT) is always the same, so why not compute it just once?

  7. There doesn't seem to be any need for separate __init__ and from_bytes methods. You only use __init__ in the test cases. So I would replace __init__ with from_bytes and adjust the test cases accordingly.

4. Data class

  1. It's not clear to me what the purpose of this class is. What kind of thing does a Data object represent? As far as I can see, the only purpose of this class is to produce a list of DataTile objects from a <data> element. So why not just write a function?

  2. The properties Data.encoding and Data.compression are buggy: you need to explicitly return the value.

  3. But why bother with properties at all? In Python, properties are normally used in three scenarios. First, to provide a simple interface to a complex data structure. Second, to give the implementer freedom to change the implementation in future. Third, in combination with setters when writing a public API whose internal data structures have an invariant that needs to be preserved. But it's not clear to me that any of those scenarios apply to your code, so why not keep it simple and just use attributes?

  4. @staticmethod is usually an anti-pattern in Python. When code has nothing to do with a class, then there's no need to make it a static method, you can just make it a plain function instead. Python is not Java (not everything needs to be part of a class).

  5. It's not clear why Data.__init__ takes compression or encoding values: these are never used.

  6. There seems to be some confusion in this class about global_tile_ids versus tiles.

  7. Data.__init__ takes a parameter with a default value of an empty list: global_tile_ids=[]. Default values for function parameters are evaluated only once, when the function is defined. So all instances of Data will share the same default value for global_tile_ids.

    If this isn't what you want, you should create a new empty list for each instance:

    def __init__(..., global_tile_ids=None, ...):
        # ...
        self.global_tile_ids = global_tile_ids or []
    
  8. Instead of:

    element.attrib['encoding'] if 'encoding' in element.attrib else None
    

    use the dict.get method and write:

    element.attrib.get('encoding')
    
  9. This code doesn't work:

    map(tile_ids.append, element.iter('tile'))
    

    There are two problems. First, map returns an iterator and does not actually apply the function until you iterate over it. Second, even if you do iterate, you'll get a list of <tile> elements, whereas what you want is a list of global ids.

    So write instead:

    tile_ids = [tile.attrib['gid'] for tile in element.iter('tile')]
    
  10. This:

    lambda x: gzip.decompress(data)
    

    looks like a mistake for:

    lambda x: gzip.decompress(x)
    

    but this is just the same as:

    gzip.decompress
    

    (which computer scientists call η-equivalence).

  11. I think you've misunderstood the TMX specification. Or at least, let us say, the TMX specification is very unclear. You've written, "Data is compressed first, then encoded" but that can't be right when encoding=csv because the types don't match. Compression outputs a byte stream, but CSV encoding takes a list of gids as input. These don't match, so the combination can't make any sense.

    The only way that I can see to make sense of the specification is that there are only four possibilities:

    compression  encoding  gids represented as
    -----------  --------  -----------------------------------------------
    None         None      gid= attributes in <tile> elements
    None         'csv'     comma-separated numbers, encoded in ASCII
    'gzip'       'base64'  32-bit numbers, gzip-compressed, base64-encoded
    'zlib'       'base64'  32-bit numbers, zlib-compressed, base64-encoded
    

    It might be worth opening an issue on GitHub and see if Thorbjørn Lindeijer can clarify the documentation.

  12. You don't properly decode the tile data! After decompressing it you just call

    tile_ids = list(decompressed)
    

    but this results in tile_ids being a list with one element for each byte in the decompressed data. But in the TMX documentation it says that these bytes "should be interpreted as an array of unsigned 32-bit integers using little-endian byte ordering." You have completely omitted this step.

5. Revised code

import base64
import gzip
import struct
import zlib

class Tile(object):
    """An instance of a tile in a map."""

    HORIZONTAL_FLIP = 1 << 31
    VERTICAL_FLIP   = 1 << 30
    DIAGONAL_FLIP   = 1 << 29
    TILE_ID_MASK    = ~(HORIZONTAL_FLIP + VERTICAL_FLIP + DIAGONAL_FLIP)

    def __init__(self, gid):
        self.global_tile_id = gid & self.TILE_ID_MASK
        self.flipped_horizontally = bool(gid & self.HORIZONTAL_FLIP)
        self.flipped_vertically = bool(gid & self.VERTICAL_FLIP)
        self.flipped_diagonally = bool(gid & self.DIAGONAL_FLIP)

def parse_data(element):
    """Parse a <data> element from Tiled Map Format (TMX) and return a
    list of Tile objects.

    """
    assert(element.tag == 'data')
    encoding = element.attrib.get('encoding')
    compression = element.attrib.get('compression')
    if encoding not in (None, 'base64', 'csv'):
        raise ValueError('unsupported encoding={}'.format(encoding))
    if compression not in (None, 'gzip', 'zlib'):
        raise ValueError('unsupported compression={}'.format(compression))
    if not encoding and not compression:
        return [Tile(int(tile.attrib['gid'])) for tile in element.iter('tile')]
    elif encoding == 'csv' and not compression:
        gids = [int(gid) for gid in data.split(',')]
    elif encoding == 'base64':
        data = base64.b64decode(element.text)
        if compression == 'gzip':
            data = gzip.decompress(data)
        elif compression == 'zlib':
            data = zlib.decompress(data)
        gids = struct.unpack('<{}L'.format(len(data) // 4), data)
    else:
        raise ValueError('unsupported combination: encoding={} compression={}'
                         .format(encoding, compression))
    return [Tile(gid) for gid in gids]
\$\endgroup\$
  • \$\begingroup\$ Wow, that is a lot to take in. Incredible work, Gareth! I've read through it once and agree with most of your points. I've only one question regarding the value_equality decorator. Wouldn't the readability of a class being decorated be the same as that of a class inherting ValueEquality? @value_equality class Foo: and class Foo(ValueEquality): seem equally readable. In any case, thank you so much for the review! I'll read through it again and refactor my implementation accordingly :) \$\endgroup\$ – IAE Dec 31 '13 at 15:23
  • 1
    \$\begingroup\$ Yes, when you come to use it there's no advantage in readability. But the implementation of a class tends to be more readable than the implementation of a decorator, so it's best to reserve decorators for when a class won't do. \$\endgroup\$ – Gareth Rees Dec 31 '13 at 15:26

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.