HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

General feedback on ChangeableRange object

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
feedbackchangeablerangeobjectgeneral

Problem

My code is below. List anything that looks wrong, anything that could be improved. This is an update from this question. I know, my old code was a complete mess.

New Code:

```
import math
class ChangeableRange:
"""With this type of range object, one
can edit the start, stop, and step for the range, as well as easily
retrieving the values in the range

"""
def __init__(self, maximum, start=0, step=1):
"""Sets the maximum, start, and step"""
try:
self.maximum = math.ceil(maximum)
self.start = math.ceil(start)
self.step = math.ceil(step)
except TypeError:
return "Error, attributes must be of type int or float"
def __iter__(self):
"""Iterates over the range"""
return iter(range(self.start, self.maximum, self.step))
def __str__(self):
"""Returns a string representation of the CRange"""
return "CRange object with Max: {0}, Min: {1}, and Step: {2}".format(self.maximum, self.start, self.step)
def __len__(self):
"""Returns the number of values in the range"""
return len(range(self.start, self.maximum, self.step))
def to_dict(self):
"""Returns a dictionary representation of the maximum, minimum, and step"""
return {"Max": self.maximum, "Min": self.start, "Step": self.step}
def change_range(self, attr, option, x):
"""Provides options to change attributes of the range"""
if option == "add" and attr == "maximum":
try:
self.maximum += x
except TypeError:
print("Error, you cannot add", x, "to the maximum, a float or integer is required")
elif option == "sub" and attr == "maximum":
try:
self.maximum -=x
except TypeError:
print("Error, you cannot subtract", x, "from the maximum, a float or integer is required")
elif option == "mult" and attr == "maximum":

Solution

One thing I believe is missing is checking whether or not the inputs are valid. The step should not be zero, for instance. To simplify maintenance, a single "setter" method can be used, so you can call it both from __init__ and change_range.

I see you do a few checks in the change_range (for instance if the step is positive), but I see two problems with that: 1) sometimes you do the check before adjusting the value, so it can still become invalid (ex.: if you pass "step", "mult", 0). Using a setter would ensure that anytime the values are set the necessary conditions are met; 2) range support negative steps, as long as maximum is smaller than start. You might want to support that too.

In your __len__ method:

def __len__(self):
    """Returns the number of values in the range"""
    return len(range(self.start, self.maximum, self.step))


Wouldn't it be more efficient to compute the number of elements, instead of creating the range and checking its length? Like this:

if (self.maximum - self.start) * self.step < 0:
    return 0 # To be consistent with range
return (self.maximum - self.start) // self.step


About the change_range function, I see you're supporting several operations (add, sub, mult, div) and applying them to each attribute (maximum, start, step). Instead of doing all the combinations, why don't you separate the concerns? Check type first, choose operation next, apply operation and check the constraints, then set the values:

# Check the parameter type
if not isinstance(x,int) and not isinstance(x,float):
    print("Error in parameter", x,  "a float or integer is required")
    return

# Choose operation
if option == 'add':
    delta = lambda x,y: x + y
elif option == 'sub':
    delta = lambda x,y: x - y
...
else:
    print("Error in parameter", option, "invalid option")
    return

# Apply operation to the right attribute
maximum = delta(self.maximum,x) if attr == 'maximum' else self.maximum
start = delta(self.start,x) if attr == 'start' else self.start
step = delta(self.step,x) if attr == 'step' else self.step

# Check the constraints and set the new values
self.attr_setter(maximum,start,step) # This would be the "setter" I mentioned earlier


Lastly, one style issue: the range builtin, when receiving a start value, receives it as the first argument. In your code it's the second. That might be confusing to users of your class. I'd suggest inverting them (when needed):

def __init__(self, maximum, start=None, step=1):
    maximum, start = (maximum, 0) if start is None else (start, maximum)


(This is not perfect: if you call your constructor with named parameters, it will invert them against your intent; so take this last advice with a grain of salt)

Code Snippets

def __len__(self):
    """Returns the number of values in the range"""
    return len(range(self.start, self.maximum, self.step))
if (self.maximum - self.start) * self.step < 0:
    return 0 # To be consistent with range
return (self.maximum - self.start) // self.step
# Check the parameter type
if not isinstance(x,int) and not isinstance(x,float):
    print("Error in parameter", x,  "a float or integer is required")
    return

# Choose operation
if option == 'add':
    delta = lambda x,y: x + y
elif option == 'sub':
    delta = lambda x,y: x - y
...
else:
    print("Error in parameter", option, "invalid option")
    return

# Apply operation to the right attribute
maximum = delta(self.maximum,x) if attr == 'maximum' else self.maximum
start = delta(self.start,x) if attr == 'start' else self.start
step = delta(self.step,x) if attr == 'step' else self.step

# Check the constraints and set the new values
self.attr_setter(maximum,start,step) # This would be the "setter" I mentioned earlier
def __init__(self, maximum, start=None, step=1):
    maximum, start = (maximum, 0) if start is None else (start, maximum)

Context

StackExchange Code Review Q#11295, answer score: 2

Revisions (0)

No revisions yet.