Skip to main content
added 9 characters in body
Source Link
janos
  • 113.1k
  • 15
  • 154
  • 396

The name _convert_string_to_array is not intuitive: it juststhe 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.

The name _convert_string_to_array is not intuitive: it justs 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.

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.
added 254 characters in body
Source Link
janos
  • 113.1k
  • 15
  • 154
  • 396

The name _convert_string_to_array is not intuitive: it justs 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.

The name _convert_string_to_array is not intuitive: it justs 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)

The name _convert_string_to_array is not intuitive: it justs 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.
Source Link
janos
  • 113.1k
  • 15
  • 154
  • 396

The name _convert_string_to_array is not intuitive: it justs 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)