1
\$\begingroup\$

Any suggestions to make this better?

    import math
    # BSD License:
    """Copyright (c) 2012, Solomon Wise
    All rights reserved.

    Redistribution and use in source and binary forms, with or without
    modification, are permitted provided that the following conditions are met:
    1. Redistributions of source code must retain the above copyright
       notice, this list of conditions and the following disclaimer.
    2. Redistributions in binary form must reproduce the above copyright
       notice, this list of conditions and the following disclaimer in the
       documentation and/or other materials provided with the distribution.
    3. All advertising materials mentioning features or use of this software
       must display the following acknowledgement:
       This product includes software developed by Solomon Wise.
    4. Neither the name of CRange nor the
       names of its contributors may be used to endorse or promote products
       derived from this software without specific prior written permission.

    THIS SOFTWARE IS PROVIDED BY SOLOMON WISE "AS IS" AND ANY
    EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
    WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
    DISCLAIMED. IN NO EVENT SHALL SOLOMON WISE BE LIABLE FOR ANY
    DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
    (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
    LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
    ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
    (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
    SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
    """
    class CRange:
        """CRange is short for Changeable Range. 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 maximumimum"""
            self.maximum = math.ceil(maximum)
            self.start = math.floor(start)
            self.step = math.ceil(step)
        def __iter__(self):
            """Iterates over the range"""
            for x in range(self.start, self.maximum, self.step):
                yield x
        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 __int__(self):
            """Returns a integer representation of the maximum"""
            return self.maximum
        def __len__(self):
            """Returns the number of values in the range"""
            x = 0
            for i in self:
                x += 1
            return x
        def tuple(self):
            """Returns a tuple representation of the maximum, minimum, and step"""
            return (self.maximum, self.start, self.step)
        def dict(self):
            """Returns a dictionary representation of the maximum, minimum, and step"""
            return {"Max": self.maximum, "Min": self.start, "Step": self.step}
        def getmaximum(self):
            """Gets the length of the range"""
            return self.maximum
        def getstart(self):
            """Gets the starting point of the range"""
            return self.start
        def getstep(self):
            """Gets the step of the range"""
            return self.step
        def getall(self):
            """Returns all range attributes"""
            print("Start:", self.start)
            print("Step:", self.step)
            print("Stop:", self.maximum)
            return 0
        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:
                    print("Error")
            elif option == "sub" and attr == "maximum":
                try:
                    self.maximum -=x
                except:
                    print("Error")
            elif option == "mult" and attr == "maximum":
                try:
                    if self.maximum <= 0:
                        raise SyntaxError
                    self.maximum *= x
                except:
                    print("Error")
            elif option == "div" and attr == "maximum":
                try:
                    self.maximum /= x
                    if self.maximum <= 0:
                        raise SyntaxError
                except:
                    print("Error")
            elif option == "add" and attr == "start":
                try:
                    self.start += x
                except:
                    print("Error")
            elif option == "sub" and attr == "start":
                try:
                    self.start -=x
                except:
                    print("Error")
            elif option == "mult" and attr == "start":
                try:
                    if self.start <= 0:
                        raise SyntaxError
                    self.start *= x
                except:
                    print("Error")
            elif option == "div" and attr == "start":
                try:
                    self.start /= x
                    if self.start <= 0:
                        raise SyntaxErros
                except:
                    print("Error")
            elif option == "add" and attr == "step":
                try:
                    self.step += x
                except:
                    print("Error")
            elif option == "sub" and attr == "step":
                try:
                    self.step -= x
                except:
                    print("Error")
            elif option == "mult" and attr == "step":
                try:
                    if self.step <= 0:
                        raise SyntaxError
                    self.step *= x
                except:
                    print("Error")
            elif option == "div" and attr == "step":
                try:
                    self.step /= x
                    if self.step <= 0:
                        raise SyntaxError
                except:
                    print("Error")
            self.start = math.ceil(self.start)
            self.step = math.ceil(self.step)
            self.maximum = math.ceil(self.maximum)
        def shift(self, x):
            """Shifts both the maximum and the start in one function call"""
            try:
                if self.maximum + x < 0 or self.start + x < 0:
                    raise SyntaxError
                self.maximum += x
                self.start += x
                self.maximum, self.start = math.ceil(self.maximum), math.ceil(self.start)
            except:
                print("Error")
        def stretch(self, x):
            """Changes both the maximum and the step in one function call"""
            try:
                if self.maximum + x < 0 or self.step + x < 0:
                    raise SyntaxError
                self.maximum += x
                self.step += x
                self.maximum, self.step = math.ceil(self.maximum), math.ceil(self.step)
            except:
                print("Error")
    def docs():
        """Documentation for the module"""
        print("This module was written for Python 3.2")
        print("Text Editors Used: Emacs, Komodo Edit")
        print("OS Used: Mac OS X")

A follow-up to this question can be found here.

\$\endgroup\$
  • \$\begingroup\$ Better in what way? Be specific. \$\endgroup\$ – Jeff Mercado Apr 28 '12 at 19:19
  • 2
    \$\begingroup\$ @JeffMercado, without any specific requests, the assumption of Code REview is that they want general feedback on the code. \$\endgroup\$ – Winston Ewert Apr 28 '12 at 19:33
  • \$\begingroup\$ @Winston: I don't know about that, we could give a better review if we were given some direction on where he wants to go. "I want this to be faster," "Is my implementation efficient," "I want to use the X design pattern" is a lot better than "How can I make this better?" \$\endgroup\$ – Jeff Mercado Apr 28 '12 at 19:35
  • 2
    \$\begingroup\$ Drop the copyright would you? NASA will not use this code and neither will Goldman Sachs. Python code is not meant to take up pages and pages ... \$\endgroup\$ – Leonid Apr 28 '12 at 20:11
5
\$\begingroup\$
import math
# BSD License:
"""Copyright (c) 2012, Solomon Wise
All rights reserved.

Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions are met:
1. Redistributions of source code must retain the above copyright
   notice, this list of conditions and the following disclaimer.
2. Redistributions in binary form must reproduce the above copyright
   notice, this list of conditions and the following disclaimer in the
   documentation and/or other materials provided with the distribution.
3. All advertising materials mentioning features or use of this software
   must display the following acknowledgement:
   This product includes software developed by Solomon Wise.
4. Neither the name of CRange nor the
   names of its contributors may be used to endorse or promote products
   derived from this software without specific prior written permission.

THIS SOFTWARE IS PROVIDED BY SOLOMON WISE "AS IS" AND ANY
EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
DISCLAIMED. IN NO EVENT SHALL SOLOMON WISE BE LIABLE FOR ANY
DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
"""

class CRange:

Is abbreviating really helpful? Maybe the name should be ChangeableRange or MutableRange. CRange is kinda problematic because C gets used to mean class in prefixe a lot

    """CRange is short for Changeable Range. 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 maximumimum"""

That's not a very good description

        self.maximum = math.ceil(maximum)
        self.start = math.floor(start)
        self.step = math.ceil(step)

Thats a seriously unexpected transformation. Using this will be confusing

    def __iter__(self):
        """Iterates over the range"""
        for x in range(self.start, self.maximum, self.step):
            yield x

Just return iter(range(self.start, self.maximum, self.step)), don't needlessly iterate over the range yourself.

    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 __int__(self):
        """Returns a integer representation of the maximum"""
        return self.maximum

Does converting a range into an int actually make any sense?

    def __len__(self):
        """Returns the number of values in the range"""
        x = 0
        for i in self:
            x += 1
        return x

That's a seriously inefficient way to calculate the length

    def tuple(self):
        """Returns a tuple representation of the maximum, minimum, and step"""
        return (self.maximum, self.start, self.step)
    def dict(self):
        """Returns a dictionary representation of the maximum, minimum, and step"""
        return {"Max": self.maximum, "Min": self.start, "Step": self.step}

Why? Why would you want either of these functions

    def getmaximum(self):
        """Gets the length of the range"""
        return self.maximum
    def getstart(self):
        """Gets the starting point of the range"""
        return self.start
    def getstep(self):
        """Gets the step of the range"""
        return self.step

Python doesn't need getter functions. Just access attribute directly or via properties.

    def getall(self):
        """Returns all range attributes"""

No, it does not.

        print("Start:", self.start)
        print("Step:", self.step)
        print("Stop:", self.maximum)
        return 0

What possible purpose would this function serve?

    def change_range(self, attr, option, x):
        """Provides options to change attributes of the range"""

What the fried turkey are you trying to do here? The whole function is a complete and total disaster. Do you seriously want this function? Whatever you are trying to do with it, you are doing it wrong.

    def shift(self, x):
        """Shifts both the maximum and the start in one function call"""
        try:
            if self.maximum + x < 0 or self.start + x < 0:
                raise SyntaxError

Raise appropriate errors. Whatever the problem is, its not a SyntaxError.

            self.maximum += x
            self.start += x
            self.maximum, self.start = math.ceil(self.maximum), math.ceil(self.start)
        except:
            print("Error")

NEVER NEVER NEVER NEVER catch errors and just print "Error." Always catch only the specific errors you want to handle, and always provide all the information available about the problem.

    def stretch(self, x):
        """Changes both the maximum and the step in one function call"""
        try:
            if self.maximum + x < 0 or self.step + x < 0:
                raise SyntaxError
            self.maximum += x
            self.step += x
            self.maximum, self.step = math.ceil(self.maximum), math.ceil(self.step)
        except:
            print("Error")

Again, don't do that.

def docs():
    """Documentation for the module"""
    print("This module was written for Python 3.2")
    print("Text Editors Used: Emacs, Komodo Edit")
    print("OS Used: Mac OS X")

Documentation should go into the docstring for the file. Not in a docs function.

This class seems very odd. I suspect that there is probably a much better way of doing whatever you are trying to do. But since you don't tell us what you are trying to do, I can't say for certain.

\$\endgroup\$
  • \$\begingroup\$ +1 I really need to post some of my python code sometime to get one of your great in-depth reviews. \$\endgroup\$ – seand Apr 29 '12 at 1:46
0
\$\begingroup\$

I mostly agree with Winston Ewert that there's likely a better way. In case there's not, here's something to cut your SLOC way down:

def change_range(self, attr, option, x):
    val = getattr(self, attr)
    op = { "add" : lambda v: v += x,
           "sub" : lambda v: v -= x,
           "div" : lambda v: v /= x,
           "mult" : lambda v: v *= x }[option]
    try:
        val = op(val)
    except Error as e:
        print("Error in change_range %s %s %d: %s" % (attr, option, x, e))
    if val <= 0:
        raise SyntaxError
    setattr(self, attr, math.ceil(val))

It's not an exact workalike (your odd error-checking for values of val is in a slightly different place in some cases) but it's pretty close. Tweak it as needed, of course.

\$\endgroup\$

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.