4
\$\begingroup\$

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":
            try:
                if self.maximum <= 0:
                    raise Exception
                self.maximum *= x
            except TypeError:
                print("Error, you cannot multiply the maximum by", x,  "a float or integer is required")
            except Exception:
                print("Error, the maximum cannot be less than or equal to zero")
        elif option == "div" and attr == "maximum":
            try:
                self.maximum /= x
                if self.maximum <= 0:
                    raise SyntaxError
            except TypeError:
                print("Error, you cannot divide the maximum by", x,  "a float or integer is required")
            except Exception:
                print("Error, the maximum cannot be less than or equal to zero")
        elif option == "add" and attr == "start":
            try:
                self.start += x
            except TypeError:
                print("Error, you cannot add", x, "to the start, a float or integer is required")
        elif option == "sub" and attr == "start":
            try:
                self.start -=x
            except TypeError:
                print("Error, you cannot subtract", x, "from the start, a float or integer is required")
        elif option == "mult" and attr == "start":
            try:
                if self.start <= 0:
                    raise SyntaxError
                self.start *= x
            except TypeError:
                print("Error, you cannot multiply the start by", x,  "a float or integer is required")
            except Exception:
                print("Error, the start cannot be less than or equal to zero")
        elif option == "div" and attr == "start":
            try:
                self.start /= x
                if self.start <= 0:
                    raise SyntaxErros
            except TypeError:
                print("Error, you cannot divide the start by", x,  "a float or integer is required")
            except Exception:
                print("Error, the start cannot be less than or equal to zero")
        elif option == "add" and attr == "step":
            try:
                self.step += x
            except TypeError:
                print("Error, you cannot add", x, "to the step, a float or integer is required")
        elif option == "sub" and attr == "step":
            try:
                self.step -= x
            except TypeError:
                print("Error, you cannot subtract", x, "from the start, a float or integer is required")
        elif option == "mult" and attr == "step":
            try:
                if self.step <= 0:
                    raise SyntaxError
                self.step *= x
            except TypeError:
                print("Error, you cannot multiply the step by", x,  "a float or integer is required")
            except Exception:
                print("Error, the step cannot be less than or equal to zero")
        elif option == "div" and attr == "step":
            try:
                self.step /= x
                if self.step <= 0:
                    raise SyntaxError
            except TypeError:
                print("Error, you cannot divide the step by", x,  "a float or integer is required")
            except Exception:
                print("Error, the step cannot be less than or equal to zero")
        self.start = math.ceil(self.start)
        self.step = math.ceil(self.step)
        self.maximum = math.ceil(self.maximum)
\$\endgroup\$
  • \$\begingroup\$ I'd be rather curious to see how you plan to use this code, especially change_range \$\endgroup\$ – Winston Ewert Apr 29 '12 at 14:18
  • 3
    \$\begingroup\$ You really should apply all suggestions that were given to you in your previous questions before coming in and asking about the same code again... a lot still hasn't changed and we're just repeating ourselves now... \$\endgroup\$ – Jeff Mercado Apr 29 '12 at 20:17
  • 1
    \$\begingroup\$ And I repeat, we really need to know how you are going to use this class. \$\endgroup\$ – Winston Ewert Apr 30 '12 at 4:35
2
\$\begingroup\$

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)

\$\endgroup\$
2
\$\begingroup\$

range() only accepts integers as input. math.ceil returns floats. You'll have to convert them to ints before calling range()

Rather than print-int errors, you should raise custom exceptions. Change all of your print("blah blahs") to raise Exception("blah blahs")

In __init__, it's unneccesary to use a try...except block as math.ceil already raises an error for incompatible types.

\$\endgroup\$
1
\$\begingroup\$
  • Read pep-8 and apply it
  • Use % instead of str.format, it's faster.
  • this can die with a division by zero:

    return int((self.maximum-self.start)/self.step)

  • Use instanceof instead of type().

  • Don't raise Exception. Use ValueError instead.
  • I suggest you make use of assert.
  • You don't need to_dict method. Just use obj.__dict__ it will return the same thing.
  • Do you really need iter?
\$\endgroup\$
0
\$\begingroup\$

I would also suggest that you should inherit your class from object as per the new style class conventions.

eg: class ChangeableRange(object)

\$\endgroup\$
  • \$\begingroup\$ In Python 3, which the OP is using (note the print('...') function rather than print '...' statement), all classes are new-style and specifying object explicitly is neither necessary nor encouraged. \$\endgroup\$ – Adam Apr 1 '13 at 11:43
  • \$\begingroup\$ Hmm, you are right! \$\endgroup\$ – vivek Apr 2 '13 at 11:25

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.