3
\$\begingroup\$

Some background:

I'm writing a number of classes that ensure user input is properly validated, and is rejected if it does not conform to expected types (there's also some extra options such as setting bounds and finite sets that the values can come from). There are a number of basic types (Integral, Boolean, String, Double, Complex), and also an Array type that uses one of these types as a "base type" (so you might have an Array of Integers for example; in this case, every value in the array must be an Integer).

For the Array type, this checking is done by instantiating it with a given base type; all values for the Array are then checked with the base type. Conversions are done by setting the value property for the given base type, (so calling self._base_type.value = x will convert x to a given type, raising a ValueError if it fails).

These types above all inherit from a _Leaf class, but that can be fairly safely ignored for this code.

Finally, each element knows how to convert itself to an XML representation.

I'm aware that this is more work than what should be done in a property, but other project restrictions effectively force me to keep it this way. For a 1-D array [1,2,3] this might look like:

<DoubleArray description="" units="" name="ArrayLeaf">
    <value>1.0</value>
    <value>2.0</value>
    <value>3.0</value>
</DoubleArray>

For a 2D array [[1,2,3],[4,5,6]]:

<DoubleArray description="" units="" name="ArrayLeaf">
    <dimension>
        <value>1.0</value>
        <value>2.0</value>
        <value>3.0</value>
    </dimension>
    <dimension>
        <value>4.0</value>
        <value>5.0</value>
        <value>6.0</value>
    </dimension>
</DoubleArray>

I'm looking for anything that cleans this up a bit (or any other feedback). Note that the XML creation uses Qt, hence the camelCase method names.

import string

class ArrayLeaf(_Leaf):
    '''Basic Leaf type for an Array. The array must be given its own
    leaf type (for example, DoubleLeaf) to be used. There are a few extra
    restrictions placed on an array type:
        - For Double/Integral types, no min/max bounds can be set.
        - For String types, no enumerations can be set.
    '''
    def __init__(self, base_leaf, name='ArrayLeaf', parent=None):
        super(ArrayLeaf, self).__init__(name, parent)
        self._base_type = base_leaf
        self._base = self._base_type._base + 'Array'
        self._values = []
        self._dimension = 0

        if not isinstance(self._base_type, _Leaf):
            raise ValueError(
                'Base leaf type for an ArrayLeaf must be derived from _Leaf')

    @property
    def dimension(self):
        return self._dimension

    @property
    def value(self):
        return self._values
    @value.setter
    def value(self, vals):
        '''Attempts to convert the given value in "vals" to a list of the
        given base_type. If any of the conversions fail, a ValueError is
        raised.
        Note that this will accept either a list or a string delimited
        by ",", surrounded by (optional) square brackets ("[]"). Extra
        dimensions are added via `;`, like in MATLAB. Only one dimensional
        lists are accepted, for 2D and above, strings delimited by ';' must be
        used.

        Example:

            a = ArrayLeaf(IntegralLeaf())
            a.value = [1, 2, 3]   # Ok, list type
            a.value = '[1,2,3]'   # Ok, comma separated string surrounded by '[]'
            a.value = '1,2,3'     # Ok, comma separated string
            a.value = [1.5, 2, 3]  # Error, 1.5 is not integral

            a.value = '1,2;3,4'    # Ok, gives 2D matrix [[1,2],[3,4]]
        '''
        if isinstance(vals, list):
            for val in vals:
                self._base_type.value = val
                self._values.append(self._base_type.value)
        elif isinstance(vals, (str, unicode)):
            vals = vals.strip('[]').strip(string.whitespace)
            self._convert_string_to_array(vals)

    def _convert_string_to_array(self, vals):
        '''Converts a string representation of a (possibly multidimensional)
        array into a Python list structure.
        '''
        self._dimension = 0
        self._values = []
        vals = vals.split(';')

        for dimension in vals:
            dimension = dimension.split(',')
            d = []
            for d_val in dimension:
                try:
                    self._base_type.value = d_val.strip(string.whitespace)
                    d.append(self._base_type.value)
                except ValueError as val_error:
                    self._values = []
                    self._dimension = 0
                    raise val_error
            self._values.append(d)
        self._dimension = len(vals)

        # Flatten the resulting list if it only has a dimension of 1.
        if self._dimension == 1:
            self._values = [item for sublist in self._values for item in sublist]

    @override(_Leaf)
    def _to_xml(self, document, parent):
        '''Produces the XML representation of the given ArrayLeaf object,
        appending it as a child to the given parent, and adding to the given
        XML document in "document".
        '''
        elem = document.createElement(self._base)
        elem.setAttribute('name', self.name)
        if self._dimension == 1:
            for val in self._values:
                value_element = document.createElement('value')
                value = document.createTextNode(str(val))
                value_element.appendChild(value)
                elem.appendChild(value_element)
        else:
            for dim in self._values:
                dimension = document.createElement('dimension')
                for val in dim:
                    value_element = document.createElement('value')
                    value = document.createTextNode(str(val))
                    value_element.appendChild(value)
                    dimension.appendChild(value_element)
                elem.appendChild(dimension)

        elem.setAttribute('description', self.description)
        elem.setAttribute('units', units_to_string(self.units))
        parent.appendChild(elem)
\$\endgroup\$
2
\$\begingroup\$

The name _convert_string_to_array is not intuitive: the name suggests a converter function that takes some input and returns converted output, but in this code it changes internal state. I'd call it _set_from_string or something similar.


try:
    self._base_type.value = d_val.strip(string.whitespace)
    d.append(self._base_type.value)
except ValueError as val_error:
    self._values = []
    self._dimension = 0
    raise val_error

Essentially you're catching the exception just to reset ._values and ._dimension in case they have been modified. It would be simpler to not modify them at all until the loop is finished, that way you won't need to catch anything:

self._values = []
self._dimension = 0
vals = vals.split(';')

new_values = []

for dimension in vals:
    dimension = dimension.split(',')
    d = []
    for d_val in dimension:
        self._base_type.value = d_val.strip(string.whitespace)
        d.append(self._base_type.value)
    new_values.append(d)

self._values = new_values
self._dimension = len(vals)

The loops inside the if-else are crying for extraction to a common method:

if self._dimension == 1:
    for val in self._values:
        value_element = document.createElement('value')
        value = document.createTextNode(str(val))
        value_element.appendChild(value)
        elem.appendChild(value_element)
else:
    for dim in self._values:
        dimension = document.createElement('dimension')
        for val in dim:
            value_element = document.createElement('value')
            value = document.createTextNode(str(val))
            value_element.appendChild(value)
            dimension.appendChild(value_element)
        elem.appendChild(dimension)

Minor things:

  • There should be a blank line before the @value.setter (before every method definition)
  • It seems to me that .strip(string.whitespace) is equivalent to .strip(). Unless I'm overlooking something, I'd use the simplest way.
\$\endgroup\$
  • \$\begingroup\$ Yep, agreed on all counts. Now to go and actually fix all that up... \$\endgroup\$ – Yuushi Sep 12 '14 at 4:39

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.